Initial import/export/self-name support by weswigham · Pull Request #68 · weswigham/TypeScript

weswigham · 2021-09-15T02:20:26Z

This is the final part of the node12 module resolution work - the new resolver itself. It has support for package exports, imports and package self-name lookup, in addition to implementing the rules for both the cjs and esm resolution modes (eg, no index or automatic extension resolution in esm flavored imports).

Also containing extensive refactoring around our resolution caching infrastructure to handle how the same import (eg, "./foo" could be resolved in one of two modes (the cjs or esm resolution mode), and can have separate resolution results for both).

TODO:

More test coverage of resolver features operating simultaneously (eg, * patterns with package imports - should work, just no tests to that effect)
More test coverage for soon-to-be-deprecated and thus supposedly node12-only features ( module: deprecate trailing slash pattern mappings nodejs/node#40039 )
typesVersions conditions and tests (an unversioned types condition is already supported) per the conclusion in Design Meeting Notes, 5/14/2021 microsoft/TypeScript#44097
Implementation reviews 🐱

…olution modes

…ptimize import name format detection by relying on parents being set

…ome reason

… import when choosing if they provide extensions

andrewbranch · 2021-09-22T17:19:47Z

src/compiler/moduleNameResolver.ts

+
+        function getUnderlyingCacheKey(specifier: string, mode: ModuleKind.CommonJS | ModuleKind.ESNext | undefined) {
+            const result = mode === undefined ? specifier : `${mode}|${specifier}`;
+            memoizedReverseKeys.set(result, [specifier, mode]);


I’m still nervous about every cache access now also doing a set in another map. Would it at least be possible to skip this under the old module resolution settings? In those cases, mode will always be undefined, so forEach wouldn’t need to do the lookup and therefore we wouldn’t need to do the set?

No? Then I'd need the compiler options in the cache key/arguments and I'd need to take that in as a parameter and switch on that and... It doesn't really change much. Plus, I really don't think this is going to matter much - this doesn't increase the complexity of the operation at all (there's no extra iteration being done) and only increases the memory usage by the size of a few pointers per element. This is highly unlikely to be any kind of bottleneck, imo.

Then I'd need the compiler options in the cache key

The cache can be used across different programs?

I should be able to test this with --extendedDiagnostics... I don’t think our perf suite reports module resolution time (or program construction time) directly, IIRC.

The cache can be used across different programs?

Right, exactly - I'd be trading the one thing I needed a reverse lookup cache for for two (since it's not like I can guarantee the new resolution modes won't ever be used in a context prior to cache construction). It wouldn't really help - the key is already minimal.

andrewbranch

I won't block on the map thing, it's something that can be optimized later if needed

sandersn

Here's what I have so far. Nothing substantive since I've just started on the bulk of the new stuff in moduleNameResolver.

Also, my two primary work computers are only able to display lines about half as long as the longest length. Please break up the lines more! In the meantime I guess I can open the review on two screens at once or something.

src/compiler/checker.ts

src/compiler/moduleNameResolver.ts

sandersn · 2021-09-22T23:51:16Z

src/compiler/moduleNameResolver.ts

+        );
+    }
+
+    function nodeNextModuleNameResolverWorker(features: NodeResolutionFeatures, moduleName: string, containingFile: string, compilerOptions: CompilerOptions, host: ModuleResolutionHost, cache?: ModuleResolutionCache, redirectedReference?: ResolvedProjectReference, resolutionMode?: ModuleKind.CommonJS | ModuleKind.ESNext): ResolvedModuleWithFailedLookupLocations {


why does node12 delegate to nodeNext...Worker instead of node...Worker or something?

nodeNext...Worker sets the appropriate EsmMode Feature flag using the resolutionMode parameter before calling into node...Worker (which no longer uses resolutionMode).

src/compiler/moduleNameResolver.ts

sandersn · 2021-09-22T23:59:35Z

src/compiler/moduleNameResolver.ts

+        }
+
+        return loadModuleFromFileNoImplicitExtensions(extensions, candidate, onlyRecordFailures, state);
+    }


can you move this closer to its usage, after getLoadModuleFromTargetImportOrExport?

Its primary usage is immediately above this in loadFromFile (which is in turn already used all over) - in getLoadModuleFromTargetImportOrExport it's a secondary, new usage?

loadJSOrExactTSFileName looks like it's new in the review. I think I put my comment on a confusing line that indicated a different function.

sandersn

I haven't looked at the tests yet, but the rest of the code looks like it matches what we've discussed in various meeting (as well as I understand it). So far I just have a couple of questions and readability suggestions, nothing substantive.

sandersn · 2021-09-23T16:57:49Z

src/compiler/moduleNameResolver.ts

+        }
+
+        return loadModuleFromFileNoImplicitExtensions(extensions, candidate, onlyRecordFailures, state);
+    }


loadJSOrExactTSFileName looks like it's new in the review. I think I put my comment on a confusing line that indicated a different function.

sandersn · 2021-09-23T17:38:46Z

src/compiler/moduleNameResolver.ts

+    function loadModuleFromImportsOrExports(extensions: Extensions, state: ModuleResolutionState, cache: ModuleResolutionCache | undefined, redirectedReference: ResolvedProjectReference | undefined, moduleName: string, lookupTable: object, scope: PackageJsonInfo, isImports: boolean): SearchResult<Resolved> | undefined {
+        const loadModuleFromTargetImportOrExport = getLoadModuleFromTargetImportOrExport(extensions, state, cache, redirectedReference, moduleName, scope, isImports);
+
+        if (!endsWith(moduleName, directorySeparator) && moduleName.indexOf("*") === -1 && hasProperty(lookupTable, moduleName)) {


lookupTable is cast to {[x: string]: unknown} 4 out of 5 times. Maybe that should be its type and the callers should cast? Or maybe the cast should happen once at the beginning?

Nah - it's possibly null or a string earlier on as well - we just don't have a "dictionary-like object" guard, so there's casts.

sandersn · 2021-09-23T17:41:06Z

src/compiler/moduleNameResolver.ts

+            return toSearchResult(/*value*/ undefined);
+        }
+        const useCaseSensitiveFileNames = typeof state.host.useCaseSensitiveFileNames === "function" ? state.host.useCaseSensitiveFileNames() : state.host.useCaseSensitiveFileNames;
+        const directoryPath = toPath(combinePaths(directory, "dummy"), state.host.getCurrentDirectory?.(), createGetCanonicalFileName(useCaseSensitiveFileNames === undefined ? true : useCaseSensitiveFileNames));


why is 'dummy' added after directory?

Because the function expects a file in the directory for the path and not a directory path - the first thing it does is pop off that last segment. We do similar elsewhere in the resolver.

sandersn · 2021-09-23T17:42:18Z

src/compiler/moduleNameResolver.ts

+            const target = (lookupTable as {[idx: string]: unknown})[moduleName];
+            return loadModuleFromTargetImportOrExport(target, /*subpath*/ "", /*pattern*/ false);
+        }
+        const expandingKeys = sort(filter(getOwnKeys(lookupTable as MapLike<unknown>), k => k.indexOf("*") !== -1 || endsWith(k, "/")), (a, b) => a.length - b.length);


can we use String.includes/String.endsWith yet? MDN says ES2015. (and we use startsWith/endsWith in the checker)

Our built-in endsWith function uses string.prototype.endsWith if it's available.

sandersn · 2021-09-23T17:55:20Z

src/compiler/moduleNameResolver.ts

+            }
+        }
+
+        function matchesPatternWithTrailer(target: string, name: string) {


This would be easier to read if all 3 cases moved their detection AND extraction code into this function, and the containing loop's body was something like loadModuleFromTargetImportOrExport(...matchesPatternWithTrailer(lookupTable, potentialTarget)).

matchesPatternWithTrailer's code could be easier to read if it handled cases in order ['fixed', 'trailing*', 'pattern*trailer'].

Most of the current export/import map lookup structure is structured like it is because it matches how the node spec says it should be structured - departing too much from that is going to make it harder to update or debug future changes, I think.

sandersn · 2021-09-23T18:00:54Z

src/compiler/moduleNameResolver.ts

+                    }
+                }
+            }
+            else if (target === null) { // eslint-disable-line no-null/no-null


you could drop the target !== null above if you move this inside the typeof target === 'object' branch

See prior comment on "it's this order because the node resolver spec days it's in this order".

sandersn · 2021-09-23T18:03:03Z

src/compiler/moduleNameResolver.ts

+            if (!startsWith(key, "types@")) return false;
+            const range = VersionRange.tryParse(key.substring("types@".length));
+            if (!range) return false;
+            return range.test(version);


where is version declared?

In core.ts. I'd normally write ts.version to clarify, but our lint settings don't like that, so... 🤷

sandersn · 2021-09-23T18:19:42Z

src/compiler/program.ts

+            return isImportCall(walkUpParenthesizedExpressions(usage.parent)) ? ModuleKind.ESNext : ModuleKind.CommonJS;
+        }
+        // in esm files, import=require statements are cjs format, otherwise everything is esm
+        // imports are only parent'd up to their containing declaration/expression, so access farther parents with care


👍 for this comment

The "imports are parent's up to their containing declaration" is a new change I made, btw, because it was hideously complicated to calculate this without those parent pointers (I did for awhile - it was expensive and bug prone - better to just set the parents when the imports are collected unconditionally).

sandersn

Just a couple of ideas for better wording. I'm not sure they're improvements however.

sandersn · 2021-09-23T20:58:16Z

tests/baselines/reference/nodeModules1(module=node12).errors.txt

+    // These should _mostly_ work - `import = require` always desugars to require calls, which do have extension and index resolution (but can't load anything that resolves to esm!)
+    import m24 = require("./");
+                         ~~~~
+!!! error TS1471: Module './' cannot be imported using this construct. The specifier only resolves to an es module, which cannot be imported synchronously. Use dynamic import instead.


It might be better to uppercase ES in the error. I'm not sure.

👍 I requested this change on a different error message in one of the other PRs. Let’s use capital ES throughout.

sandersn · 2021-09-23T22:23:49Z

tests/baselines/reference/nodeModulesAllowJsPackageExports(module=node12).errors.txt

+    import * as cjsi from "inner/cjs";
+    import * as mjsi from "inner/mjs";
+                          ~~~~~~~~~~~
+!!! error TS1471: Module 'inner/mjs' cannot be imported using this construct. The specifier only resolves to an es module, which cannot be imported synchronously. Use dynamic import instead.


'statement' reads better than 'construct' here -- it's more specific and common.

I use construct because it also applies to cjs require, which is an expression, and having two error messages just to distinguish statement vs expression seems gratuitous.

weswigham · 2021-09-24T22:58:58Z

Merged in microsoft#45884

Initial import/export/self-name support

093002c

weswigham mentioned this pull request Sep 15, 2021

moduleResolution: node12 support microsoft/TypeScript#45884

Merged

weswigham force-pushed the imports-exports-selfs branch from cea73a2 to 093002c Compare September 15, 2021 02:27

weswigham and others added 11 commits September 14, 2021 19:47

Merge branch 'mjscjs-support' into imports-exports-selfs

859abb6

TypesVersions support in export/import map conditions

c21cb48

Fix import suggestion and autoimport default extensions under new res…

9294633

…olution modes

Add tests for import maps non-relative name lookup feature

c776453

Fix isDeclarationFileName for .d.mts and .d.cts

ae83938

Preserve new extensions when generating module specifiers

8ab7517

Fix spurious implict any suggestion caused by file ordering bug and o…

222ba00

…ptimize import name format detection by relying on parents being set

Fix a bunch of incremental bugs that dont repro under fourslash for s…

408ef20

…ome reason

Merge branch 'mjscjs-support' into imports-exports-selfs

44b81b2

Always include extensions on completions for cjs/mjs style imports

4a33975

String completion relative import suggestions respect the mode of the…

76afb79

… import when choosing if they provide extensions

andrewbranch reviewed Sep 22, 2021

View reviewed changes

DanielRosenwasser mentioned this pull request Sep 22, 2021

Design Meeting Notes, 9/22/2021 microsoft/TypeScript#46012

Closed

andrewbranch approved these changes Sep 22, 2021

View reviewed changes

sandersn reviewed Sep 23, 2021

View reviewed changes

Style feedback

0891db4

sandersn reviewed Sep 23, 2021

View reviewed changes

sandersn approved these changes Sep 23, 2021

View reviewed changes

Change diagnostic case

a2e799a

weswigham closed this Sep 24, 2021

sandersn deleted the imports-exports-selfs branch October 14, 2021 15:59

Conversation

weswigham commented Sep 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

andrewbranch left a comment

Choose a reason for hiding this comment

Uh oh!

sandersn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weswigham Sep 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sandersn 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!

sandersn 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!

weswigham Sep 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

weswigham commented Sep 15, 2021 •

edited

Loading

weswigham Sep 23, 2021 •

edited

Loading

weswigham Sep 24, 2021 •

edited

Loading