Reduce file lookups in node_modules module resolution #16568

ghost · 2017-06-15T20:10:02Z

In node_modules, look for a '.d.ts' file before looking for a '.ts' file -- opposite of how it is normally.
Never look for node_modules/foo.d.ts, only node_modules/foo/index.d.ts.

Edit by @DanielRosenwasser: Addresses issues seen in #16426.

yuit · 2017-06-16T18:45:29Z

src/compiler/moduleNameResolver.ts

-        TypeScript, /** '.ts', '.tsx', or '.d.ts' */
-        JavaScript, /** '.js' or '.jsx' */
-        DtsOnly /** Only '.d.ts' */
+    const enum Extensions {


Name this enum as Extensions seems misleading. I was expecting file extensions....though I have no good idea for alternatives 😞

How about ExtensionsToTry?

Maybe ExtensionsOfKind?

On reflection, calling this ExtensionsToTry is better. Renaming instances to extensionsToTry would help clarify their use as well.

ghost · 2017-06-20T20:59:17Z

@yuit @aozgaa Could you give this a review?

sheetalkamat · 2017-06-20T23:01:11Z

tests/baselines/reference/moduleResolutionWithExtensions_unexpected2.trace.json

    "Directory '/node_modules/@types' does not exist, skipping all lookups in it.",
    "Loading module 'foo' from 'node_modules' folder, target file type 'JavaScript'.",
-    "File '/node_modules/foo.js' does not exist.",
-    "File '/node_modules/foo.jsx' does not exist.",


removal of this path lookup seems incorrect.
Refer https://nodejs.org/api/modules.html

LOAD_NODE_MODULES(X, START)

let DIRS=NODE_MODULES_PATHS(START)

for each DIR in DIRS:
a. LOAD_AS_FILE(DIR/X)
b. LOAD_AS_DIRECTORY(DIR/X)

It looks like node does look for node_modules/foo.js

That's true, but as far as I know, every package manager fills node_modules with only directories, and searching for a file there is costing us a lot of fileExists calls (as you can see from the diff).

Pinging @DanielRosenwasser , as he may have some more insight into whether we want to do this.

Its unlikely we will break anyone with this change, but I'd like to see what kind of performance gain we get out of removing these extraneous lookups.

I ran the perf tests and there is basically no change between this branch and master -- a 0.5% improvement in parse time.

aozgaa

From a technical perspective, looks good modulo the naming of Extensions and some minor nits.

Still not sure we want to go ahead and make this change to explicitly drop supporting node-modules/foo.ts as a target of module resolution.

Can you test the effect of the change on RWC?

aozgaa · 2017-06-21T17:50:30Z

src/compiler/moduleNameResolver.ts

@@ -920,22 +939,26 @@ namespace ts {
        return combinePaths(directory, "package.json");
    }

-    function loadModuleFromNodeModulesFolder(extensions: Extensions, moduleName: string, nodeModulesFolder: string, nodeModulesFolderExists: boolean, failedLookupLocations: Push<string>, state: ModuleResolutionState): Resolved | undefined {
-        const candidate = normalizePath(combinePaths(nodeModulesFolder, moduleName));
+    // Inside of node_modules we should be looking for '.d.ts' files before '.ts' files.


Perhaps:

Inside node_modules, we should look for '.d.ts' ...

aozgaa · 2017-06-21T17:53:05Z

src/compiler/moduleNameResolver.ts

    }

    function loadModuleFromNodeModules(extensions: Extensions, moduleName: string, directory: string, failedLookupLocations: Push<string>, state: ModuleResolutionState, cache: NonRelativeModuleNameResolutionCache): SearchResult<Resolved> {
-        return loadModuleFromNodeModulesWorker(extensions, moduleName, directory, failedLookupLocations, state, /*typesOnly*/ false, cache);
+        // In node_modules, we will look for a '.d.ts' file first.


Considering what the next line says, not sure what this comment adds.

aozgaa · 2017-06-21T17:55:25Z

src/compiler/moduleNameResolver.ts

-    function loadModuleFromNodeModulesFolder(extensions: Extensions, moduleName: string, nodeModulesFolder: string, nodeModulesFolderExists: boolean, failedLookupLocations: Push<string>, state: ModuleResolutionState): Resolved | undefined {
-        const candidate = normalizePath(combinePaths(nodeModulesFolder, moduleName));
+    // Inside of node_modules we should be looking for '.d.ts' files before '.ts' files.
+    type NodeModulesExtensions = Extensions.TypeScriptDtsFirst | Extensions.JavaScript | Extensions.DtsOnly;


I would move this type declaration next to the enum declaration for Extensions.

aozgaa · 2017-06-21T17:58:48Z

src/compiler/moduleNameResolver.ts

-        TypeScript, /** '.ts', '.tsx', or '.d.ts' */
-        JavaScript, /** '.js' or '.jsx' */
-        DtsOnly /** Only '.d.ts' */
+    const enum Extensions {


On reflection, calling this ExtensionsToTry is better. Renaming instances to extensionsToTry would help clarify their use as well.

aozgaa · 2017-06-21T18:00:53Z

src/compiler/moduleNameResolver.ts

@@ -32,10 +32,27 @@ namespace ts {
     * Kinds of file that we are currently looking for.
     * Typically there is one pass with Extensions.TypeScript, then a second pass with Extensions.JavaScript.


Add a comment explaining the significance of the enum's members' values.

This is pretty heavily commented already, what should I add?

aozgaa · 2017-06-21T18:02:18Z

src/compiler/moduleNameResolver.ts

+         * '.ts', '.tsx', or '.d.ts'
+         * This is for regular imports of TypeScript code.
+         */
+        TypeScript = "TypeScript",


Would adding ('.ts' preferred) make sense here? May clarify debugging for people looking at trace-reoslution output.

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

@Andy-MS please refresh this PR.

typescript-bot · 2018-06-13T17:11:12Z

Thanks for your contribution. This PR has not been updated in a while and cannot be automatically merged at the time being. For housekeeping purposes we are closing stale PRs. If you'd still like to continue working on this PR, please leave a message and one of the maintainers can reopen it.

msftclas added the cla-not-required label Jun 15, 2017

Reduce file lookups in node_modules module resolution

a316827

ghost force-pushed the dts-first branch from 2acedd6 to a316827 Compare June 15, 2017 20:48

ghost requested a review from aozgaa June 16, 2017 17:49

yuit reviewed Jun 16, 2017

View reviewed changes

sheetalkamat reviewed Jun 20, 2017

View reviewed changes

aozgaa reviewed Jun 21, 2017

View reviewed changes

Andy Hanson added 2 commits June 23, 2017 09:09

Rename Extensions -> ExtensionsToTry

80bbeea

Update baselines

ed30611

ghost force-pushed the dts-first branch from 2738728 to ed30611 Compare June 26, 2017 22:01

Andy Hanson added 2 commits July 14, 2017 12:08

Merge branch 'master' into dts-first

60eee20

Merge branch 'master' into dts-first

2564ca6

msftgits removed the cla-not-required label Sep 26, 2017

Merge branch 'master' into dts-first

5140312

ghost force-pushed the dts-first branch from 62240ee to 5140312 Compare January 12, 2018 18:55

typescript-bot closed this Jun 13, 2018

jakebailey deleted the dts-first branch November 7, 2022 17:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce file lookups in node_modules module resolution #16568

Reduce file lookups in node_modules module resolution #16568

ghost commented Jun 15, 2017 •

edited by DanielRosenwasser

Loading

yuit Jun 16, 2017

ghost Jun 16, 2017

aozgaa Jun 20, 2017

aozgaa Jun 21, 2017

ghost commented Jun 20, 2017

sheetalkamat Jun 20, 2017

ghost Jun 21, 2017

aozgaa Jun 21, 2017

rbuckton Jul 14, 2017

ghost Jul 17, 2017

aozgaa left a comment

aozgaa Jun 21, 2017

aozgaa Jun 21, 2017

aozgaa Jun 21, 2017

aozgaa Jun 21, 2017

aozgaa Jun 21, 2017

ghost Jun 23, 2017

aozgaa Jun 21, 2017

mhegazy commented Jan 8, 2018

typescript-bot commented Jun 13, 2018

		@@ -32,10 +32,27 @@ namespace ts {
		* Kinds of file that we are currently looking for.
		* Typically there is one pass with Extensions.TypeScript, then a second pass with Extensions.JavaScript.

Reduce file lookups in node_modules module resolution #16568

Reduce file lookups in node_modules module resolution #16568

Conversation

ghost commented Jun 15, 2017 • edited by DanielRosenwasser Loading

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 Jun 20, 2017

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

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

mhegazy commented Jan 8, 2018

typescript-bot commented Jun 13, 2018

ghost commented Jun 15, 2017 •

edited by DanielRosenwasser

Loading