Skip to content
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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 10 additions & 8 deletions src/compiler/moduleNameResolver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1708,7 +1708,8 @@ namespace ts {
return idx === -1 ? { packageName: moduleName, rest: "" } : { packageName: moduleName.slice(0, idx), rest: moduleName.slice(idx + 1) };
}

function allKeysStartWithDot(obj: MapLike<unknown>) {
/* @internal */
export function allKeysStartWithDot(obj: MapLike<unknown>) {
return every(getOwnKeys(obj), k => startsWith(k, "."));
}

Expand Down Expand Up @@ -1922,14 +1923,15 @@ namespace ts {
}
return toSearchResult(/*value*/ undefined);
}
}

function isApplicableVersionedTypesKey(conditions: string[], key: string) {
if (conditions.indexOf("types") === -1) return false; // only apply versioned types conditions if the types condition is applied
if (!startsWith(key, "types@")) return false;
const range = VersionRange.tryParse(key.substring("types@".length));
if (!range) return false;
return range.test(version);
}
/* @internal */
export function isApplicableVersionedTypesKey(conditions: string[], key: string) {
if (conditions.indexOf("types") === -1) return false; // only apply versioned types conditions if the types condition is applied
if (!startsWith(key, "types@")) return false;
const range = VersionRange.tryParse(key.substring("types@".length));
if (!range) return false;
return range.test(version);
}

function loadModuleFromNearestNodeModulesDirectory(extensions: Extensions, moduleName: string, directory: string, state: ModuleResolutionState, cache: ModuleResolutionCache | undefined, redirectedReference: ResolvedProjectReference | undefined): SearchResult<Resolved> {
Expand Down
93 changes: 90 additions & 3 deletions src/compiler/moduleSpecifiers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Member Author

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.

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 };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

}
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) };
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

Choose a reason for hiding this comment

The 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) {
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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
Copy link
Member Author

Choose a reason for hiding this comment

The 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 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.

Copy link
Member

Choose a reason for hiding this comment

The 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;
Expand All @@ -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)) {
Expand Down
97 changes: 97 additions & 0 deletions src/testRunner/unittests/tsbuild/moduleSpecifiers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -88,4 +88,101 @@ namespace ts {
commandLineArgs: ["-b", "/src", "--verbose"]
});
});

// https://github.com/microsoft/TypeScript/issues/44434 but with `module: node12`, some `exports` maps blocking direct access, and no `baseUrl`
describe("unittests:: tsbuild:: moduleSpecifiers:: synthesized module specifiers across referenced projects resolve correctly", () => {
verifyTsc({
scenario: "moduleSpecifiers",
subScenario: `synthesized module specifiers across projects resolve correctly`,
fs: () => loadProjectFromFiles({
"/src/src-types/index.ts": Utils.dedent`
export * from './dogconfig.js';`,
"/src/src-types/dogconfig.ts": Utils.dedent`
export interface DogConfig {
name: string;
}`,
"/src/src-dogs/index.ts": Utils.dedent`
export * from 'src-types';
export * from './lassie/lassiedog.js';
`,
"/src/src-dogs/dogconfig.ts": Utils.dedent`
import { DogConfig } from 'src-types';

export const DOG_CONFIG: DogConfig = {
name: 'Default dog',
};
`,
"/src/src-dogs/dog.ts": Utils.dedent`
import { DogConfig } from 'src-types';
import { DOG_CONFIG } from './dogconfig.js';

export abstract class Dog {

public static getCapabilities(): DogConfig {
return DOG_CONFIG;
}
}
`,
"/src/src-dogs/lassie/lassiedog.ts": Utils.dedent`
import { Dog } from '../dog.js';
import { LASSIE_CONFIG } from './lassieconfig.js';

export class LassieDog extends Dog {
protected static getDogConfig = () => LASSIE_CONFIG;
}
`,
"/src/src-dogs/lassie/lassieconfig.ts": Utils.dedent`
import { DogConfig } from 'src-types';

export const LASSIE_CONFIG: DogConfig = { name: 'Lassie' };
`,
"/src/tsconfig-base.json": Utils.dedent`
{
"compilerOptions": {
"declaration": true,
"module": "node12"
}
}`,
"/src/src-types/package.json": Utils.dedent`
{
"type": "module",
"exports": "./index.js"
}`,
"/src/src-dogs/package.json": Utils.dedent`
{
"type": "module",
"exports": "./index.js"
}`,
"/src/src-types/tsconfig.json": Utils.dedent`
{
"extends": "../tsconfig-base.json",
"compilerOptions": {
"composite": true
},
"include": [
"**/*"
]
}`,
"/src/src-dogs/tsconfig.json": Utils.dedent`
{
"extends": "../tsconfig-base.json",
"compilerOptions": {
"composite": true
},
"references": [
{ "path": "../src-types" }
],
"include": [
"**/*"
]
}`,
}, ""),
modifyFs: fs => {
fs.writeFileSync("/lib/lib.es2020.full.d.ts", tscWatch.libFile.content);
fs.symlinkSync("/src", "/src/src-types/node_modules");
fs.symlinkSync("/src", "/src/src-dogs/node_modules");
},
commandLineArgs: ["-b", "src/src-types", "src/src-dogs", "--verbose"]
});
});
}
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

Loading