Follow and respect export maps when generating module specifiers #46159
Conversation
typescript-bot
added
Author: Team
For Uncommitted Bug
| Pattern | ||
| } | ||
|
|
||
| function tryGetModuleNameFromExports(options: CompilerOptions, targetFilePath: string, packageDirectory: string, packageName: string, exports: unknown, conditions: string[], mode = MatchingMode.Exact): { moduleFileToTry: string } | undefined { |
There was a problem hiding this comment.
This is essentially recurring over the whole exports object until it hits something that resolves to the file path we want a specifier for, and then returns it. Multiple mappings could exist for a given target path (eg, {"./index": "./index.js", ".": "./index.js"}) - right now, this just returns the first; but this can be swapped to a generator in a pretty straightforward way if we ever wanna explore more possibilities (and use some heuristic to rank them) in the future.
| const packageRootPath = path.substring(0, packageRootIndex); | ||
| const packageJsonPath = combinePaths(packageRootPath, "package.json"); | ||
| let moduleFileToTry = path; | ||
| if (host.fileExists(packageJsonPath)) { | ||
| const packageJsonContent = JSON.parse(host.readFile!(packageJsonPath)!); | ||
| // TODO: Inject `require` or `import` condition based on the intended import mode |
There was a problem hiding this comment.
Specifier generation takes configuration information that's too far removed from what we need to make an intelligent choice here - we need to know if the specifier's going to be used in a cjs mode or esm mode import location (which is keyed off the syntax for the import and the mode of the containing file for that import), but all we have are specifier preferences like "Minimal" or "Extension". I have yet to go through and rejigger every specifier generator caller to calculate the mode of the import it's generating and pass that in, so for now we're just not passing either condition. If one would be required to reach a file, we'll fail to find it via exports, block loading it via the package file, and then return a relative node_modules path, which we'll then later throw on in declaration emit as "non-portable" and request a type annotation. I think that's pretty fair for now while we still work on the API refactoring here.
There was a problem hiding this comment.
Does one of the existing tests show an example of this?
weswigham
force-pushed
the
specifiers-across-projects
branch
from
aa1bc52 to
a7cbfe4
Compare
weswigham
force-pushed
the
specifiers-across-projects
branch
from
a7cbfe4 to
7929c5a
Compare
| switch (mode) { | ||
| case MatchingMode.Exact: | ||
| if (comparePaths(targetFilePath, pathOrPattern) === Comparison.EqualTo || (extensionSwappedTarget && comparePaths(extensionSwappedTarget, pathOrPattern) === Comparison.EqualTo)) { | ||
| return { moduleFileToTry: packageName }; |
There was a problem hiding this comment.
I find moduleFileToTry a bit of a confusing name, as it looks like it’s generally not very filey—it often ends up being just the package name, or an entry in an export map, right? Should it just be moduleSpecifierToTry?
Also, what does the “trying”? Is the “try” because it might be a path to a file that gets blocked by exports?
| const packageRootPath = path.substring(0, packageRootIndex); | ||
| const packageJsonPath = combinePaths(packageRootPath, "package.json"); | ||
| let moduleFileToTry = path; | ||
| if (host.fileExists(packageJsonPath)) { | ||
| const packageJsonContent = JSON.parse(host.readFile!(packageJsonPath)!); | ||
| // TODO: Inject `require` or `import` condition based on the intended import mode |
There was a problem hiding this comment.
Does one of the existing tests show an example of this?
| const trailingSlice = pathOrPattern.slice(starPos + 1); | ||
| if (startsWith(targetFilePath, leadingSlice) && endsWith(targetFilePath, trailingSlice)) { | ||
| const starReplacement = targetFilePath.slice(leadingSlice.length, targetFilePath.length - trailingSlice.length); | ||
| return { moduleFileToTry: packageName.replace("*", starReplacement) }; |
There was a problem hiding this comment.
So CodeQL is unhappy with this - is that legit? Or does the format not allow multiple patterns?
There was a problem hiding this comment.
Export maps don't allow multiple * - for once I actually wanted the default replace behavior. So I marked the codeQLs as "won't fix".
|
@typescript-bot pack this |
|
Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at 1fe1769. You can monitor the build here. |
|
Cancelled it. I think I'll pull in the PR as-is, but getting a test in would be ideal. |
This partially fixes one of the known limitations of declaration emit for the
module: node12emit mode(s). With this, our specifier generation code (used both both declaration emit and auto imports) will followexportsmaps when looking for specifiers to use.importsand package self-names are still unused, however (but I believe in all cases those can be replaced safely with relative paths or other package paths, so don't have to be used - unlikeexportswhich blocks external access to files other than through its aliases).