-
Notifications
You must be signed in to change notification settings - Fork 12.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Follow and respect export maps when generating module specifiers #46159
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -555,6 +555,77 @@ namespace ts.moduleSpecifiers { | |
} | ||
} | ||
|
||
const enum MatchingMode { | ||
Exact, | ||
Directory, | ||
Pattern | ||
} | ||
|
||
function tryGetModuleNameFromExports(options: CompilerOptions, targetFilePath: string, packageDirectory: string, packageName: string, exports: unknown, conditions: string[], mode = MatchingMode.Exact): { moduleFileToTry: string } | undefined { | ||
if (typeof exports === "string") { | ||
const pathOrPattern = getNormalizedAbsolutePath(combinePaths(packageDirectory, exports), /*currentDirectory*/ undefined); | ||
const extensionSwappedTarget = hasTSFileExtension(targetFilePath) ? removeFileExtension(targetFilePath) + tryGetJSExtensionForFile(targetFilePath, options) : undefined; | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I find Also, what does the “trying”? Is the “try” because it might be a path to a file that gets blocked by |
||
} | ||
break; | ||
case MatchingMode.Directory: | ||
if (containsPath(pathOrPattern, targetFilePath)) { | ||
const fragment = getRelativePathFromDirectory(pathOrPattern, targetFilePath, /*ignoreCase*/ false); | ||
return { moduleFileToTry: getNormalizedAbsolutePath(combinePaths(combinePaths(packageName, exports), fragment), /*currentDirectory*/ undefined) }; | ||
} | ||
break; | ||
case MatchingMode.Pattern: | ||
const starPos = pathOrPattern.indexOf("*"); | ||
const leadingSlice = pathOrPattern.slice(0, starPos); | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So CodeQL is unhappy with this - is that legit? Or does the format not allow multiple patterns? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Export maps don't allow multiple * - for once I actually wanted the default replace behavior. So I marked the codeQLs as "won't fix". |
||
} | ||
if (extensionSwappedTarget && startsWith(extensionSwappedTarget, leadingSlice) && endsWith(extensionSwappedTarget, trailingSlice)) { | ||
const starReplacement = extensionSwappedTarget.slice(leadingSlice.length, extensionSwappedTarget.length - trailingSlice.length); | ||
return { moduleFileToTry: packageName.replace("*", starReplacement) }; | ||
} | ||
break; | ||
} | ||
} | ||
else if (Array.isArray(exports)) { | ||
return forEach(exports, e => tryGetModuleNameFromExports(options, targetFilePath, packageDirectory, packageName, e, conditions)); | ||
} | ||
else if (typeof exports === "object" && exports !== null) { // eslint-disable-line no-null/no-null | ||
if (allKeysStartWithDot(exports as MapLike<unknown>)) { | ||
// sub-mappings | ||
// 3 cases: | ||
// * directory mappings (legacyish, key ends with / (technically allows index/extension resolution under cjs mode)) | ||
// * pattern mappings (contains a *) | ||
// * exact mappings (no *, does not end with /) | ||
return forEach(getOwnKeys(exports as MapLike<unknown>), k => { | ||
const subPackageName = getNormalizedAbsolutePath(combinePaths(packageName, k), /*currentDirectory*/ undefined); | ||
const mode = endsWith(k, "/") ? MatchingMode.Directory | ||
: stringContains(k, "*") ? MatchingMode.Pattern | ||
: MatchingMode.Exact; | ||
return tryGetModuleNameFromExports(options, targetFilePath, packageDirectory, subPackageName, (exports as MapLike<unknown>)[k], conditions, mode); | ||
}); | ||
} | ||
else { | ||
// conditional mapping | ||
for (const key of getOwnKeys(exports as MapLike<unknown>)) { | ||
if (key === "default" || conditions.indexOf(key) >= 0 || isApplicableVersionedTypesKey(conditions, key)) { | ||
const subTarget = (exports as MapLike<unknown>)[key]; | ||
const result = tryGetModuleNameFromExports(options, targetFilePath, packageDirectory, packageName, subTarget, conditions); | ||
if (result) { | ||
return result; | ||
} | ||
} | ||
} | ||
} | ||
} | ||
return undefined; | ||
} | ||
|
||
function tryGetModuleNameFromRootDirs(rootDirs: readonly string[], moduleFileName: string, sourceDirectory: string, getCanonicalFileName: (file: string) => string, ending: Ending, compilerOptions: CompilerOptions): string | undefined { | ||
const normalizedTargetPath = getPathRelativeToRootDirs(moduleFileName, rootDirs, getCanonicalFileName); | ||
if (normalizedTargetPath === undefined) { | ||
|
@@ -586,7 +657,15 @@ namespace ts.moduleSpecifiers { | |
let moduleFileNameForExtensionless: string | undefined; | ||
while (true) { | ||
// If the module could be imported by a directory name, use that directory's name | ||
const { moduleFileToTry, packageRootPath } = tryDirectoryWithPackageJson(packageRootIndex); | ||
const { moduleFileToTry, packageRootPath, blockedByExports, verbatimFromExports } = tryDirectoryWithPackageJson(packageRootIndex); | ||
if (getEmitModuleResolutionKind(options) !== ModuleResolutionKind.Classic) { | ||
if (blockedByExports) { | ||
return undefined; // File is under this package.json, but is not publicly exported - there's no way to name it via `node_modules` resolution | ||
} | ||
if (verbatimFromExports) { | ||
return moduleFileToTry; | ||
} | ||
} | ||
if (packageRootPath) { | ||
moduleSpecifier = packageRootPath; | ||
isPackageRootPath = true; | ||
|
@@ -621,12 +700,21 @@ namespace ts.moduleSpecifiers { | |
// For classic resolution, only allow importing from node_modules/@types, not other node_modules | ||
return getEmitModuleResolutionKind(options) === ModuleResolutionKind.Classic && packageName === nodeModulesDirectoryName ? undefined : packageName; | ||
|
||
function tryDirectoryWithPackageJson(packageRootIndex: number) { | ||
function tryDirectoryWithPackageJson(packageRootIndex: number): { moduleFileToTry: string, packageRootPath?: string, blockedByExports?: true, verbatimFromExports?: true } { | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does one of the existing tests show an example of this? |
||
const fromExports = packageJsonContent.exports && typeof packageJsonContent.name === "string" ? tryGetModuleNameFromExports(options, path, packageRootPath, packageJsonContent.name, packageJsonContent.exports, ["node", "types"]) : undefined; | ||
if (fromExports) { | ||
const withJsExtension = !hasTSFileExtension(fromExports.moduleFileToTry) ? fromExports : { moduleFileToTry: removeFileExtension(fromExports.moduleFileToTry) + tryGetJSExtensionForFile(fromExports.moduleFileToTry, options) }; | ||
return { ...withJsExtension, verbatimFromExports: true }; | ||
} | ||
if (packageJsonContent.exports) { | ||
return { moduleFileToTry: path, blockedByExports: true }; | ||
} | ||
const versionPaths = packageJsonContent.typesVersions | ||
? getPackageJsonTypesVersionsPaths(packageJsonContent.typesVersions) | ||
: undefined; | ||
|
@@ -641,7 +729,6 @@ namespace ts.moduleSpecifiers { | |
moduleFileToTry = combinePaths(packageRootPath, fromPaths); | ||
} | ||
} | ||
|
||
// If the file is the main module, it can be imported by the package name | ||
const mainFileRelative = packageJsonContent.typings || packageJsonContent.types || packageJsonContent.main; | ||
if (isString(mainFileRelative)) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
tests/cases/conformance/node/index.ts(2,23): error TS2307: Cannot find module 'inner/other' or its corresponding type declarations. | ||
tests/cases/conformance/node/index.ts(3,14): error TS2742: The inferred type of 'a' cannot be named without a reference to './node_modules/inner/other.js'. This is likely not portable. A type annotation is necessary. | ||
tests/cases/conformance/node/index.ts(3,19): error TS1378: Top-level 'await' expressions are only allowed when the 'module' option is set to 'es2022', 'esnext', 'system', or 'nodenext', and the 'target' option is set to 'es2017' or higher. | ||
|
||
|
||
==== tests/cases/conformance/node/index.ts (3 errors) ==== | ||
// esm format file | ||
import { Thing } from "inner/other"; | ||
~~~~~~~~~~~~~ | ||
!!! error TS2307: Cannot find module 'inner/other' or its corresponding type declarations. | ||
export const a = (await import("inner")).x(); | ||
~ | ||
!!! error TS2742: The inferred type of 'a' cannot be named without a reference to './node_modules/inner/other.js'. This is likely not portable. A type annotation is necessary. | ||
~~~~~ | ||
!!! error TS1378: Top-level 'await' expressions are only allowed when the 'module' option is set to 'es2022', 'esnext', 'system', or 'nodenext', and the 'target' option is set to 'es2017' or higher. | ||
==== tests/cases/conformance/node/node_modules/inner/index.d.ts (0 errors) ==== | ||
// esm format file | ||
export { x } from "./other.js"; | ||
==== tests/cases/conformance/node/node_modules/inner/other.d.ts (0 errors) ==== | ||
// esm format file | ||
export interface Thing {} | ||
export const x: () => Thing; | ||
==== tests/cases/conformance/node/package.json (0 errors) ==== | ||
{ | ||
"name": "package", | ||
"private": true, | ||
"type": "module", | ||
"exports": "./index.js" | ||
} | ||
==== tests/cases/conformance/node/node_modules/inner/package.json (0 errors) ==== | ||
{ | ||
"name": "inner", | ||
"private": true, | ||
"type": "module", | ||
"exports": "./index.js" | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
//// [tests/cases/conformance/node/nodeModulesExportsBlocksSpecifierResolution.ts] //// | ||
|
||
//// [index.ts] | ||
// esm format file | ||
import { Thing } from "inner/other"; | ||
export const a = (await import("inner")).x(); | ||
//// [index.d.ts] | ||
// esm format file | ||
export { x } from "./other.js"; | ||
//// [other.d.ts] | ||
// esm format file | ||
export interface Thing {} | ||
export const x: () => Thing; | ||
//// [package.json] | ||
{ | ||
"name": "package", | ||
"private": true, | ||
"type": "module", | ||
"exports": "./index.js" | ||
} | ||
//// [package.json] | ||
{ | ||
"name": "inner", | ||
"private": true, | ||
"type": "module", | ||
"exports": "./index.js" | ||
} | ||
|
||
//// [index.js] | ||
export const a = (await import("inner")).x(); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
=== tests/cases/conformance/node/index.ts === | ||
// esm format file | ||
import { Thing } from "inner/other"; | ||
>Thing : Symbol(Thing, Decl(index.ts, 1, 8)) | ||
|
||
export const a = (await import("inner")).x(); | ||
>a : Symbol(a, Decl(index.ts, 2, 12)) | ||
>(await import("inner")).x : Symbol(x, Decl(index.d.ts, 1, 8)) | ||
>"inner" : Symbol("tests/cases/conformance/node/node_modules/inner/index", Decl(index.d.ts, 0, 0)) | ||
>x : Symbol(x, Decl(index.d.ts, 1, 8)) | ||
|
||
=== tests/cases/conformance/node/node_modules/inner/index.d.ts === | ||
// esm format file | ||
export { x } from "./other.js"; | ||
>x : Symbol(x, Decl(index.d.ts, 1, 8)) | ||
|
||
=== tests/cases/conformance/node/node_modules/inner/other.d.ts === | ||
// esm format file | ||
export interface Thing {} | ||
>Thing : Symbol(Thing, Decl(other.d.ts, 0, 0)) | ||
|
||
export const x: () => Thing; | ||
>x : Symbol(x, Decl(other.d.ts, 2, 12)) | ||
>Thing : Symbol(Thing, Decl(other.d.ts, 0, 0)) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
=== tests/cases/conformance/node/index.ts === | ||
// esm format file | ||
import { Thing } from "inner/other"; | ||
>Thing : any | ||
|
||
export const a = (await import("inner")).x(); | ||
>a : import("tests/cases/conformance/node/node_modules/inner/other").Thing | ||
>(await import("inner")).x() : import("tests/cases/conformance/node/node_modules/inner/other").Thing | ||
>(await import("inner")).x : () => import("tests/cases/conformance/node/node_modules/inner/other").Thing | ||
>(await import("inner")) : typeof import("tests/cases/conformance/node/node_modules/inner/index") | ||
>await import("inner") : typeof import("tests/cases/conformance/node/node_modules/inner/index") | ||
>import("inner") : Promise<typeof import("tests/cases/conformance/node/node_modules/inner/index")> | ||
>"inner" : "inner" | ||
>x : () => import("tests/cases/conformance/node/node_modules/inner/other").Thing | ||
|
||
=== tests/cases/conformance/node/node_modules/inner/index.d.ts === | ||
// esm format file | ||
export { x } from "./other.js"; | ||
>x : () => import("tests/cases/conformance/node/node_modules/inner/other").Thing | ||
|
||
=== tests/cases/conformance/node/node_modules/inner/other.d.ts === | ||
// esm format file | ||
export interface Thing {} | ||
export const x: () => Thing; | ||
>x : () => Thing | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.