Get packageId for relative import within a package #21130

ghost · 2018-01-10T21:54:46Z

Fixes #20945 (tested with the sample repository provided)

If we're loading relatively from a file, we will parse the result path to see if we're currently in a node_modules; if so, we will look for a packageId for the current package.

amcasey

Seems reasonable.

amcasey · 2018-01-10T23:48:44Z

src/compiler/moduleNameResolver.ts

@@ -816,6 +818,33 @@ namespace ts {
        return loadNodeModuleFromDirectory(extensions, candidate, failedLookupLocations, onlyRecordFailures, state, considerPackageJson);
    }

+    function parseNodeModuleFromPath(path: string): { packageDirectory: string, subModuleName: string } | undefined {
+        const nodeModules = "/node_modules/";


Is this case-sensitive even on Windows?

Should be fixed if we normalize the path first.

amcasey · 2018-01-10T23:51:30Z

src/compiler/moduleNameResolver.ts

+
+    function indexOfNextSlash(s: string, pos: number): number {
+        const x = s.indexOf(directorySeparator, pos);
+        return x === -1 ? pos : x;


This special case makes sense for this application, but probably not in general. Would it make sense to move this helper into parseNodeModuleFromPath?

I'm not a fan of moving things into closures if they don't need to close over any variables, but this could be named better.

amcasey · 2018-01-10T23:52:24Z

src/compiler/moduleNameResolver.ts

+
+    function removeExtensionAndIndex(fileName: string): string {
+        const noExtension = removeFileExtension(fileName);
+        return noExtension === "index" ? "" : removeSuffix(noExtension, "/index");


Presumably, we won't reach here for (e.g.) "index.vb"?

removeFileExtension only removes the supported file extensions (and also correctly removes all of .d.ts and not just .ts. Although it's also true that fileName shouldn't have a .vb extension since we'd never resolve a module to a .vb file.

ghost · 2018-01-11T20:32:42Z

@amcasey Good to merge?

amcasey · 2018-01-11T20:40:45Z

No objections here, but it might be nice to have a second set of eyes.

ghost · 2018-01-11T22:08:28Z

@aozgaa @sheetalkamat

aozgaa

Some variable naming suggestions and questions about valid inputs.

aozgaa · 2018-01-16T18:54:16Z

src/compiler/moduleNameResolver.ts

@@ -816,6 +818,34 @@ namespace ts {
        return loadNodeModuleFromDirectory(extensions, candidate, failedLookupLocations, onlyRecordFailures, state, considerPackageJson);
    }

+    function parseNodeModuleFromPath(path: string): { packageDirectory: string, subModuleName: string } | undefined {
+        path = normalizePath(path);
+        const nodeModules = "/node_modules/";


maybe rename to nodeModulesPathPart and declare outside the function?

aozgaa · 2018-01-16T19:00:17Z

src/compiler/moduleNameResolver.ts

+        const indexAfterNodeModules = idx + nodeModules.length;
+        let indexAfterPackageName = moveToNextDirectorySeparatorIfAvailable(path, indexAfterNodeModules);
+        if (path.charCodeAt(indexAfterNodeModules) === CharacterCodes.at) {
+            indexAfterPackageName = moveToNextDirectorySeparatorIfAvailable(path, indexAfterPackageName);


Would it make sense to get a path like /node_modules/@asdf? How should we handle such an input if so?

A path should be a complete file path like /node_modules/@asdf/pkg/index.d.ts, not just /node_modules/@asdf.

Are we concerned with malformed paths and packages? As a scenario, do we care if someone is mucking about and makes a malformed entry manually? I personally think tsserver shouldn't crash, and ideally we would surface a message to a user.

Do we have some way of handling situations like this gracefully?

I personally think tsserver shouldn't crash

That's just, like, your opinion, man. 😜

But we shouldn't get a malformed path here because this is only called after a successful module resolution. And even if we did, I don't see a place where this function would crash.

aozgaa · 2018-01-16T19:03:16Z

src/compiler/moduleNameResolver.ts

+            indexAfterPackageName = moveToNextDirectorySeparatorIfAvailable(path, indexAfterPackageName);
+        }
+        const packageDirectory = path.slice(0, indexAfterPackageName);
+        const subModuleName = removeExtensionAndIndex(path.slice(indexAfterPackageName));


From the caller loadModuleFromNodeModulesFolder and the use of getPackageJsonInfo, it looks like subModuleName should refer to submodule within a nested module (eg: "core" in "@angular/core"). Is this correct?

Should subModuleName be "" when we are handling a module without submodules?

subModuleName is the name of a path within a package, so for @angular/core/foo.d.ts it would be "foo" and for @angular/core/index.d.ts it would be the empty string "".

aozgaa · 2018-01-16T19:04:23Z

src/compiler/moduleNameResolver.ts

+    }
+
+    function moveToNextDirectorySeparatorIfAvailable(path: string, pos: number): number {
+        const indexOfSlash = path.indexOf(directorySeparator, pos);


maybe indexOfNextSlash?

aozgaa · 2018-01-16T19:05:42Z

src/compiler/moduleNameResolver.ts

+        return { packageDirectory, subModuleName };
+    }
+
+    function moveToNextDirectorySeparatorIfAvailable(path: string, pos: number): number {


Should pos necessarily be the index of a slash? Could we rename the argument to slashIndex or add a comment if so?

aozgaa · 2018-01-16T20:32:50Z

src/compiler/moduleNameResolver.ts

+        return indexOfSlash === -1 ? pos : indexOfSlash;
+    }
+
+    function removeExtensionAndIndex(fileName: string): string {


Do we have a convention where fileName is a path? If it someone were to input just the last part of a path here (eg: "index.d.ts") then the result we would get would be "index". Is that okay?

I'll rename the parameter to path.

aozgaa · 2018-01-16T20:37:03Z

src/compiler/moduleNameResolver.ts

+        const indexAfterNodeModules = idx + nodeModules.length;
+        let indexAfterPackageName = moveToNextDirectorySeparatorIfAvailable(path, indexAfterNodeModules);
+        if (path.charCodeAt(indexAfterNodeModules) === CharacterCodes.at) {
+            indexAfterPackageName = moveToNextDirectorySeparatorIfAvailable(path, indexAfterPackageName);


How does the method work with submodules under @types (eg: "@types/someModule/Submodule")? Or is that not a valid input here?

In that case subModuleName would be "Submodule".

aozgaa · 2018-01-17T01:28:26Z

src/compiler/moduleNameResolver.ts

+    /**
+     * packageDirectory is the directory of the package itself.
+     * subModuleName is the path within the package.
+     *   For `foo/index.d.ts` this is { packageDirectory: "foo", subModuleName: "" }.


Should these examples start with "/node_modules/"?

mhegazy · 2018-01-17T18:06:21Z

src/compiler/moduleNameResolver.ts

+
+        const indexAfterNodeModules = idx + nodeModulesPathPart.length;
+        let indexAfterPackageName = moveToNextDirectorySeparatorIfAvailable(path, indexAfterNodeModules);
+        if (path.charCodeAt(indexAfterNodeModules) === CharacterCodes.at) {


can you add a test for scoped packages as well

mhegazy · 2018-01-17T18:07:34Z

src/compiler/moduleNameResolver.ts

+     */
+    function parseNodeModuleFromPath(path: string): { packageDirectory: string, subModuleName: string } | undefined {
+        path = normalizePath(path);
+        const idx = path.indexOf(nodeModulesPathPart);


should not this be lastIndexOf instead? you want the last node_modules in the list right?

mhegazy · 2018-01-17T18:08:50Z

tests/baselines/reference/duplicatePackage_relativeImportWithinPackage.trace.json

+    "File '/node_modules/foo/use.tsx' does not exist.",
+    "File '/node_modules/foo/use.d.ts' exist - use it as a name resolution result.",
+    "Resolving real path for '/node_modules/foo/use.d.ts', result '/node_modules/foo/use.d.ts'.",
+    "======== Module name 'foo/use' was successfully resolved to '/node_modules/foo/use.d.ts'. ========",


this is not related to this PR, but just a thought that came to me, we are not logging the packageid anywhere. we should probably to make these tests more statically verifiable

mhegazy

A few comments

Get packageId for relative import within a package

e096b08

ghost requested review from aozgaa, amcasey and sheetalkamat January 10, 2018 21:55

amcasey approved these changes Jan 10, 2018

View reviewed changes

Code review

376a320

aozgaa reviewed Jan 16, 2018

View reviewed changes

Rename things and add comments

07516ab

aozgaa reviewed Jan 17, 2018

View reviewed changes

Improve documentation

c7bde6f

ghost force-pushed the duplicatePackage_relativeImportWithinPackage branch from 095694a to c7bde6f Compare January 17, 2018 16:05

mhegazy reviewed Jan 17, 2018

View reviewed changes

mhegazy approved these changes Jan 17, 2018

View reviewed changes

ghost mentioned this pull request Jan 17, 2018

Log packageId in --traceResolution #21232

Closed

Test for scoped packages

3420bd6

ghost force-pushed the duplicatePackage_relativeImportWithinPackage branch from d5c4f2d to 3420bd6 Compare January 17, 2018 18:48

ghost merged commit 61fb845 into master Jan 17, 2018

ghost deleted the duplicatePackage_relativeImportWithinPackage branch January 17, 2018 19:14

ghost mentioned this pull request Jan 17, 2018

Use packageId for suggestion to install @types/packageName #21241

Merged

microsoft locked and limited conversation to collaborators Jul 3, 2018

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get packageId for relative import within a package #21130

Get packageId for relative import within a package #21130

ghost commented Jan 10, 2018

amcasey left a comment

amcasey Jan 10, 2018

ghost Jan 11, 2018

amcasey Jan 10, 2018

ghost Jan 11, 2018

amcasey Jan 10, 2018

ghost Jan 11, 2018

ghost commented Jan 11, 2018

amcasey commented Jan 11, 2018

ghost commented Jan 11, 2018

aozgaa left a comment

aozgaa Jan 16, 2018

ghost Jan 16, 2018

aozgaa Jan 16, 2018

ghost Jan 16, 2018

aozgaa Jan 17, 2018

ghost Jan 17, 2018

aozgaa Jan 16, 2018

ghost Jan 16, 2018

aozgaa Jan 16, 2018

aozgaa Jan 16, 2018

ghost Jan 16, 2018

aozgaa Jan 16, 2018

ghost Jan 16, 2018

aozgaa Jan 16, 2018

ghost Jan 16, 2018

aozgaa Jan 17, 2018

mhegazy Jan 17, 2018

mhegazy Jan 17, 2018

ghost Jan 17, 2018

mhegazy Jan 17, 2018

ghost Jan 17, 2018

mhegazy left a comment

Get packageId for relative import within a package #21130

Get packageId for relative import within a package #21130

Conversation

ghost commented Jan 10, 2018

amcasey left a 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

ghost commented Jan 11, 2018

amcasey commented Jan 11, 2018

ghost commented Jan 11, 2018

aozgaa left a 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

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

mhegazy left a comment

Choose a reason for hiding this comment