Skip to content

moduleSpecifiers: Simpler criteria for preferring relative path vs baseUrl #25803

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
4 commits merged into from
Aug 29, 2018

Conversation

ghost
Copy link

@ghost ghost commented Jul 19, 2018

Fixes microsoft/vscode#52485

Previously we were just checking if relativePath climbed all the way up to baseUrl. But in a case where we have /src/d0/d1/d2/d3/d4/d5/user.ts importing from src/d0/a.ts, it was only climbing up most of the way to baseUrl. pathFromSourceToBaseUrl grew just as fast as relativePath and we never switched over to using baseUrl.

New criteria is to just count the number of components in each path.

@ghost ghost requested a review from mhegazy July 20, 2018 00:47
@mhegazy
Copy link
Contributor

mhegazy commented Jul 20, 2018

@weswigham this relevant to the discussion we had earlier about comparing "better-ness".

@ghost ghost requested a review from weswigham July 28, 2018 00:04
@weswigham
Copy link
Member

Right, although in the context of declaration emit, the betterness of multiple non-relative specifiers are probably more important.

At least for declarations, I see the priority as probably module name > shortest node_modules absolute > other node_modules absolute > relative > base url absolute - the base url case moved to the end because declarations don't carry their baseUrl setting with them, so relying on it in declaration emit is liable to cause awkward names to crop up (like src/app when ../app would be better).

@ghost
Copy link
Author

ghost commented Aug 10, 2018

Latest commit fixes #26348

@ghost ghost force-pushed the countPathComponents branch from 8ad44b6 to 1f51140 Compare August 10, 2018 00:49
@ghost ghost requested review from sheetalkamat and removed request for mhegazy August 27, 2018 22:25
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants