match leading slash imports with path mappings - fixes #13730 #27980

EECOLOR · 2018-10-18T22:32:23Z

Checklist:

There is an associated issue
The issue is labeled 'Bug' or 'help wanted' or is in the Community milestone
Code is up-to-date with the master branch
You've successfully run jake runtests locally
You've signed the CLA
There are new or updated unit tests validating the change

About the code changes: I can imagine you would like to have the code structured differently or have different traces. I just looked at what currently been implemented and took the most straightforward approach making the changes easy to diff. Feel free to make suggestions.

About the tests: I am missing information to provide the correct amount of tests, or actually I don't know what your policy is. I think the tests I added should cover all of the code I added and prevent future breakage.

About performance: I tried to minimize the performance impact. I am not sure how often this code is called in situations where high performance is required. That said, I noticed a lot of places where performance could be improved, so I am not sure how much you guys want to focus on that.

About caching: I noticed a few places where isExternalModuleNameRelative is used in relation to caching. We might want to add the 'is potentially an alias import' there as well. This however requires some extra thought. An example of such a place:

if (perFolderCache) {
  perFolderCache.set(moduleName, result);
  if (!isExternalModuleNameRelative(moduleName)) {
    // put result in per-module name cache
    cache!.getOrCreateCacheForModuleName(moduleName).set(containingDirectory, result);
  }
}

Fixes #13730

msftclas · 2018-10-18T22:32:34Z

All CLA requirements met.

ajafff · 2018-10-19T07:05:53Z

src/compiler/moduleNameResolver.ts

@@ -1433,3 +1444,9 @@ namespace ts {
        return value !== undefined ? { value } : undefined;
    }
 }
+/* @internal */
+namespace ts {
+    export function startsWithSlash(name: string) {


Since this function is not used anywhere else, you could avoid exporting it

Great tip. I tried that first, but I had some trouble which seemed to be caused by the /* internal */ comment, it could not resolve CharacterCodes. I just tried to move it again, and now it works. My knowledge of TypeScript is very low, so I will adjust it.

src/compiler/moduleNameResolver.ts

EECOLOR · 2018-10-19T18:54:47Z

Question for the maintainers: do you guys want me to squash the commits once the code has stabilized?

EECOLOR · 2018-10-20T11:39:47Z

@sheetalkamat I have made some changes:

Added a test to reflect the intent expressed in the result of the design meeting
Changed the names of the tests to better reflect the intent
Adjusted the code to more clearly communicate the flow: "if we have paths available and it is eligible for path mapping, try to load using the paths"

EECOLOR · 2018-10-20T12:28:56Z

There is one potential problem left.

When thinking about this problem it is important to remember that the function isExternalModuleNameRelative means isExternalModuleNameRelativeOrRoot or isExternalModuleNameNotANodeModule'. And when you read !isExternalModuleNameRelative(moduleName)you could sayisExternalModuleNameANodeModule`.

Before this change, all !isExternalModuleNameRelative(moduleName) could potentially be mapped using paths. And, the other way around, mapped paths would always be non-root and non-relative.

With this change, all !pathIsRelative(moduleName) and all isRootedDiskPath(moduleName) || isUrl(moduleName) could potentially be mapped using paths (where the last case has the requirement that we have a potential mapping). And, the other way around, mapped paths would always be non-relative.

This is not necessarily a problem. However, in some code bases there is 'invisible' coupling between concepts. The coupling for this case would be the assumption that !isExternalModuleNameRelative(moduleName) is potentially mapped.

As far as I can tell (by tracing back the introduction of path mapping and the introduction of root path's as 'relative') any coupling that exist would be accidental. I can also imagine that if a developer used !isExternalModuleNameRelative(moduleName) to mean a node module he/she did not think about the possibility of it being a mapped path and thus potentially not a node module.

Looking at the uses of isExternalModuleNameRelative my domain knowledge is lacking to determine the following two things:

Did it contain a wrong assumption about the meaning of isExternalModuleNameRelative (in combination with paths) before this change?
Does this change impact correct assumptions about the meaning of isExternalModuleNameRelative (in combination with paths)?

My advise would be to create a separate issue to track down the uses of isExternalModuleNameRelative and:

investigate if it has any relation to node modules or rooted paths
determine if the fact that those paths might actually not be a node module or rooted path (once resolved) has any impact

EECOLOR · 2018-10-20T12:41:52Z

I have squashed the commits.

EECOLOR · 2018-10-25T11:58:00Z

@sheetalkamat What's next?

sheetalkamat · 2018-10-29T16:31:20Z

src/compiler/moduleNameResolver.ts

        if (!baseUrl) {
            return undefined;
        }
        if (state.traceEnabled) {
            trace(state.host, Diagnostics.baseUrl_option_is_set_to_0_using_this_value_to_resolve_non_relative_module_name_1, baseUrl, moduleName);
        }
-        if (paths) {


I think this should be pulled out into separate function instead of inlining. Its easier to read.
We also need trace above this line about using baseUrl in that.

I think this should be pulled out into separate function instead of inlining.

I thought about that. I however couldn't think of a name. The name tryLoadModuleUsingPaths is already taken. What do you suggest?

We also need trace above this line about using baseUrl in that.

The trace baseUrl_option_is_set_to_0_using_this_value_to_resolve_non_relative_module_name_1 is not exactly correct for what is happening. And since we have the trace paths_option_is_specified_looking_for_a_pattern_to_match_module_name_0 I thought that would be enough. But if you really want it there I could of course add it back in.

Yes please add that back in there since its used in there anyways.

sheetalkamat · 2018-10-29T16:33:19Z

src/compiler/moduleNameResolver.ts

+    function isEligibleForPathMapping(moduleName: string, paths: MapLike<string[]>): boolean {
+        if (isRootedDiskPath(moduleName) || isUrl(moduleName)) {
+            // check for potential alias otherwise { '*': './src/*' } will match
+            const root = moduleName.slice(0, getRootLength(moduleName));


You don't need all these checks. The path mapping needs to be done if: moduleName is not relative path or it is absolute path.

Did you read the comment?

It would break backwards compatibility and existing configurations.

@DanielRosenwasser isn't this change expected breaking change ?

Yes, I thought that was the point of this PR in the first place. What exactly are the extra checks meant to protect against?

Root (or url) imports failing (resolving to another file) in existing projects failing because they have a * mapping.

As mentioned this is desired to be a breaking change. Please remove these extra checks

sheetalkamat · 2018-10-29T16:38:57Z

src/compiler/moduleNameResolver.ts

+            const resolved = tryLoadModuleUsingPaths(extensions, moduleName, baseUrl, paths, loader, /*onlyRecordFailures*/ false, state);
+            if (resolved) return resolved.value;
+        }
+
        if (!isExternalModuleNameRelative(moduleName)) {


Also needs some tests to ensure that caching is working as intended (and hasn't changed)

Could you point me in the right direction? I am unsure what would be needed.

There are tests in src/testRunner/moduleResolution.ts that verify caching is working as expected.

@sheetalkamat Can you please explain how I can run the unit tests? Thank you

@sheetalkamat I figured it out, they are run by default

sheetalkamat · 2018-10-29T18:15:02Z

Also need changes in getStringLiteralCompletionsFromModuleNamesWorker and corresponding tests with module name completions..

EECOLOR · 2018-10-29T18:46:52Z

Also need changes in getStringLiteralCompletionsFromModuleNamesWorker and corresponding tests with module name completions.

What kind of inputs would this fail on?

sheetalkamat · 2018-10-29T18:54:00Z

More than failing of completions, we need to make sure the path completions are correct when there is path mapping.

EECOLOR · 2018-11-01T22:47:07Z

@sheetalkamat I have updated the code to reflect your comments (apart from the ones that still have questions). I also squashed my commits again and rebased on master. Could you please take another look?

DanielRosenwasser · 2018-11-05T22:30:46Z

src/compiler/moduleNameResolver.ts

+        if (isRootedDiskPath(moduleName) || isUrl(moduleName)) {
+            // check for potential alias otherwise { '*': './src/*' } will match
+            const root = moduleName.slice(0, getRootLength(moduleName));
+            const potentialAlias = getOwnKeys(paths).find(path => path.startsWith(root));


find and startsWith are ES2015 and we support ES5 environments, so use the helper functions from core.ts

DanielRosenwasser · 2018-11-05T22:31:17Z

src/compiler/moduleNameResolver.ts

+            // check for potential alias otherwise { '*': './src/*' } will match
+            const root = moduleName.slice(0, getRootLength(moduleName));
+            const potentialAlias = getOwnKeys(paths).find(path => path.startsWith(root));
+            return Boolean(potentialAlias);


sheetalkamat · 2018-11-07T02:57:48Z

src/services/stringCompletions.ts

-            if (compilerOptions.rootDirs) {
-                return getCompletionEntriesForDirectoryFragmentWithRootDirs(
-                    compilerOptions.rootDirs, literalValue, scriptDirectory, extensionOptions, compilerOptions, host, scriptPath);
+        if (isRootedDiskPath(literalValue) || isUrl(literalValue)) {


@andy-ms can you please review completions change

ghost · 2018-11-07T17:13:26Z

src/services/stringCompletions.ts

-                    compilerOptions.rootDirs, literalValue, scriptDirectory, extensionOptions, compilerOptions, host, scriptPath);
+        if (isRootedDiskPath(literalValue) || isUrl(literalValue)) {
+            if (compilerOptions.baseUrl) {
+                return getCompletionEntriesForNonRelativeModules(literalValue, scriptDirectory, compilerOptions, host, typeChecker);


The two getCompletionEntriesForNonRelativeModules calls are identical, as are the two getCompletionEntriesForRelativeModules calls. This could be a single conditional instead of several nested ones.

return isPathRelativeToScript(literalValue) || !compilerOptions.baseUrl && (isRootedDiskPath(literalValue) || isUrl(literalValue)) ? getCompletionEntriesForRelativeModules(literalValue, scriptDirectory, compilerOptions, host, scriptPath) : getCompletionEntriesForNonRelativeModules(literalValue, scriptDirectory, compilerOptions, host, typeChecker);

…ft#13730

EECOLOR · 2018-11-07T21:31:12Z

@sheetalkamat I have made the required changes.

sheetalkamat

What is the change in tests/cases/user/TypeScript-Node-Starter/TypeScript-Node-Starter ?? Please revert it. Thanks

ghost · 2018-11-08T16:54:52Z

@sheetalkamat Those are really difficult to revert -- it happens when the submodule is updated by git. I think it's fine because we regularly update these anyway.

sheetalkamat · 2018-11-08T17:21:25Z

@EECOLOR thank you for contribution. This should be available in tonight's nightly build.

EECOLOR · 2018-11-08T19:42:25Z

@sheetalkamat thank you and your team for your help

EECOLOR · 2018-11-09T22:21:12Z

@sheetalkamat did something go wrong with the nightly build?

sheetalkamat · 2018-11-09T22:47:20Z

@weswigham @DanielRosenwasser would know.

weswigham · 2018-11-09T23:55:44Z

Looks like for some reason the nightly publish job didn't get scheduled until I just went to go check on the task. Likely an azure devops issue - since I believe it just triggered, we'll probably publish a nightly in about 30 minutes.

Update README

ajafff reviewed Oct 19, 2018

View reviewed changes

sheetalkamat reviewed Oct 19, 2018

View reviewed changes

src/compiler/moduleNameResolver.ts Outdated Show resolved Hide resolved

RyanCavanaugh mentioned this pull request Oct 19, 2018

Design Meeting notes, 10/19/2018 #27994

Closed

EECOLOR force-pushed the leading-slash-imports branch from 2c8642e to 82e9ef7 Compare October 20, 2018 12:39

sheetalkamat reviewed Oct 29, 2018

View reviewed changes

EECOLOR force-pushed the leading-slash-imports branch 2 times, most recently from a361e89 to 340bb87 Compare November 1, 2018 22:45

DanielRosenwasser reviewed Nov 5, 2018

View reviewed changes

EECOLOR force-pushed the leading-slash-imports branch from 340bb87 to aadfa61 Compare November 6, 2018 20:44

sheetalkamat reviewed Nov 7, 2018

View reviewed changes

sheetalkamat requested a review from a user November 7, 2018 03:01

ghost reviewed Nov 7, 2018

View reviewed changes

path mappings now supports rooted disk paths and urls - fixes microso…

1a9c209

…ft#13730

EECOLOR force-pushed the leading-slash-imports branch from aadfa61 to 1a9c209 Compare November 7, 2018 21:20

sheetalkamat approved these changes Nov 8, 2018

View reviewed changes

ghost approved these changes Nov 8, 2018

View reviewed changes

sheetalkamat requested changes Nov 8, 2018

View reviewed changes

sheetalkamat approved these changes Nov 8, 2018

View reviewed changes

sheetalkamat merged commit b534fb4 into microsoft:master Nov 8, 2018

EECOLOR deleted the leading-slash-imports branch November 8, 2018 19:42

EECOLOR mentioned this pull request Nov 9, 2018

Meteor typescript support meteor/meteor-feature-requests#285

Closed

EECOLOR referenced this pull request in barbatus/typescript Jan 9, 2019

Bump version

58137c0

Update README

matthew-dean mentioned this pull request Apr 10, 2019

VSCode doesn't support leading slash imports microsoft/vscode#72093

Closed

EECOLOR mentioned this pull request Sep 5, 2019

TypeScript paths config doesn't work with leading slashes. #13730

Closed

thesunny mentioned this pull request Mar 29, 2020

The valid path "/*" does not resolve correctly dividab/tsconfig-paths#113

Closed

surgicaI mentioned this pull request Nov 19, 2020

Does not work for path mapping starting with / dividab/tsconfig-paths#128

Closed

match leading slash imports with path mappings - fixes #13730 #27980

match leading slash imports with path mappings - fixes #13730 #27980

Conversation

EECOLOR commented Oct 18, 2018 • edited Loading

msftclas commented Oct 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EECOLOR commented Oct 19, 2018

EECOLOR commented Oct 20, 2018

EECOLOR commented Oct 20, 2018

EECOLOR commented Oct 20, 2018

EECOLOR commented Oct 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sheetalkamat commented Oct 29, 2018

EECOLOR commented Oct 29, 2018

sheetalkamat commented Oct 29, 2018

EECOLOR commented Nov 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EECOLOR commented Nov 7, 2018

sheetalkamat left a comment

Choose a reason for hiding this comment

ghost commented Nov 8, 2018

sheetalkamat commented Nov 8, 2018

EECOLOR commented Nov 8, 2018

EECOLOR commented Nov 9, 2018

sheetalkamat commented Nov 9, 2018

weswigham commented Nov 9, 2018

EECOLOR commented Oct 18, 2018 •

edited

Loading

msftclas commented Oct 18, 2018 •

edited

Loading