Get packageId for relative import within a package · Pull Request #21130 · microsoft/TypeScript

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

    }

+    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

+
+    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

+
+    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


+    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

+        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

+            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

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


maybe indexOfNextSlash?

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

+        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

+        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

+        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

+    /**
+     * 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

+
+        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

+     */
+    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

+    "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 amcasey, aozgaa 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.

Conversation

ghost commented Jan 10, 2018

Uh oh!

amcasey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Jan 11, 2018

Uh oh!

amcasey commented Jan 11, 2018

Uh oh!

ghost commented Jan 11, 2018

Uh oh!

aozgaa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhegazy left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants