fix(core): scope typescript resolution cache correctly when processing the graph (#31455)
## Current Behavior When using Node.js subpath imports with the same name in different projects, the Nx graph incorrectly picks up seemingly random dependencies between projects that shouldn't exist. This happens because the result of the resolution performed with TypeScript is cached using the import path as the cache key. The problem with that is that multiple projects can have the same subpath import name pointing to internal files of the project, so when the resolution is made for the first project (say `project1`), the result will be cached and incorrectly reused for other projects with the same subpath import name. So, all projects with the same subpath import name would resolve the dependency to the first project (`project1`). The same could happen to projects with TS path mappings defined in the project's tsconfig file. These TS path mappings would only apply to the project internally and therefore, other unrelated projects could also define them with the same name pointing to different files. ## Expected Behavior The Node.js subpath imports should be handled correctly. The TypeScript resolution result should be cached safely and scoped to the project from which the import is being done. ## Related Issue(s) Fixes #31223
This commit is contained in:
Leosvel Pérez Espinosa
GitHub
parent
77ff63f356
commit
8941362d1a
@ -8,6 +8,7 @@ import {
|
||||
readJson,
|
||||
runCLI,
|
||||
runCommand,
|
||||
uniq,
|
||||
updateFile,
|
||||
updateJson,
|
||||
} from '@nx/e2e/utils';
|
||||
@ -319,6 +320,155 @@ describe('Graph - TS solution setup', () => {
|
||||
);
|
||||
});
|
||||
|
||||
it('should detect dependencies correctly when using Node.js subpath imports', () => {
|
||||
const pmc = getPackageManagerCommand();
|
||||
|
||||
const pkg1 = uniq('pkg1');
|
||||
createPackage(pkg1, {
|
||||
sourceFilePaths: [
|
||||
'src/file1.ts',
|
||||
'src/nested/file2.ts',
|
||||
'src/file3.ts',
|
||||
'src/file4.ts',
|
||||
],
|
||||
packageJsonEntryFields: {
|
||||
imports: {
|
||||
'#*': './src/*.ts',
|
||||
'#file2': './src/nested/file2.ts',
|
||||
'#file3': {
|
||||
types: './src/file3.ts',
|
||||
default: './src/file3.ts',
|
||||
},
|
||||
'#file4': 'external-package',
|
||||
},
|
||||
},
|
||||
});
|
||||
const pkg2 = uniq('pkg2');
|
||||
createPackage(pkg2, {
|
||||
sourceFilePaths: [
|
||||
'src/file1.ts',
|
||||
'src/nested/file2.ts',
|
||||
'src/file3.ts',
|
||||
'src/file4.ts',
|
||||
],
|
||||
packageJsonEntryFields: {
|
||||
imports: {
|
||||
'#*': './src/*.ts',
|
||||
'#file2': './src/nested/file2.ts',
|
||||
'#file3': {
|
||||
types: './src/file3.ts',
|
||||
default: './src/file3.ts',
|
||||
},
|
||||
'#file4': 'external-package',
|
||||
},
|
||||
},
|
||||
});
|
||||
|
||||
// create a fake external package
|
||||
createFile(
|
||||
'temp-external-package/package.json',
|
||||
JSON.stringify(
|
||||
{
|
||||
name: 'external-package',
|
||||
version: '1.0.0',
|
||||
exports: {
|
||||
'.': {
|
||||
types: './index.d.ts',
|
||||
default: './index.js',
|
||||
},
|
||||
'./package.json': './package.json',
|
||||
},
|
||||
},
|
||||
null,
|
||||
2
|
||||
)
|
||||
);
|
||||
createFile(
|
||||
'temp-external-package/index.js',
|
||||
`export const externalPackage = 'external-package';`
|
||||
);
|
||||
createFile(
|
||||
'temp-external-package/index.d.ts',
|
||||
`export const externalPackage: string;`
|
||||
);
|
||||
// create a tarball of the external package
|
||||
runCommand('cd temp-external-package && npm pack');
|
||||
// add the external package as a dependency
|
||||
updateJson('package.json', (json) => {
|
||||
json.dependencies ??= {};
|
||||
json.dependencies['external-package'] =
|
||||
'file:./temp-external-package/external-package-1.0.0.tgz';
|
||||
return json;
|
||||
});
|
||||
// install dependencies to update the lock file
|
||||
runCommand(pmc.install);
|
||||
|
||||
createFile(
|
||||
`packages/${pkg1}/src/main.ts`,
|
||||
`
|
||||
import { file1 } from '#file1';
|
||||
import { file2 } from '#file2';
|
||||
import { file3 } from '#file3';
|
||||
import { externalPackage } from '#file4';
|
||||
|
||||
export const main = file1 + file2 + file3 + externalPackage;
|
||||
`
|
||||
);
|
||||
createFile(
|
||||
`packages/${pkg2}/src/main.ts`,
|
||||
`
|
||||
import { file1 } from '#file1';
|
||||
import { file2 } from '#file2';
|
||||
import { file3 } from '#file3';
|
||||
import { externalPackage } from '#file4';
|
||||
|
||||
export const main = file1 + file2 + file3 + externalPackage;
|
||||
`
|
||||
);
|
||||
|
||||
// force the graph to be generated
|
||||
runCLI(`reset`);
|
||||
runCLI(`report`);
|
||||
const graph = readJson('.nx/workspace-data/project-graph.json');
|
||||
// only `external-package` is detected as a dependency, the rest of the
|
||||
// imports point to internal files of each project
|
||||
expect(
|
||||
graph.dependencies[`@proj/${pkg1}`].map((d) => d.target)
|
||||
).toStrictEqual(['npm:external-package']);
|
||||
expect(
|
||||
graph.dependencies[`@proj/${pkg2}`].map((d) => d.target)
|
||||
).toStrictEqual(['npm:external-package']);
|
||||
|
||||
// assert build succeeds, tsc outputs nothing when successful
|
||||
expect(runCommand(`${pmc.exec} tsc -b packages/${pkg1}`)).toBe('');
|
||||
checkFilesExist(
|
||||
`packages/${pkg1}/dist/src/file1.js`,
|
||||
`packages/${pkg1}/dist/src/file1.d.ts`,
|
||||
`packages/${pkg1}/dist/src/nested/file2.js`,
|
||||
`packages/${pkg1}/dist/src/nested/file2.d.ts`,
|
||||
`packages/${pkg1}/dist/src/file3.js`,
|
||||
`packages/${pkg1}/dist/src/file3.d.ts`,
|
||||
`packages/${pkg1}/dist/src/file4.js`,
|
||||
`packages/${pkg1}/dist/src/file4.d.ts`,
|
||||
`packages/${pkg1}/dist/src/main.js`,
|
||||
`packages/${pkg1}/dist/src/main.d.ts`
|
||||
);
|
||||
// assert build succeeds, tsc outputs nothing when successful
|
||||
expect(runCommand(`${pmc.exec} tsc -b packages/${pkg2}`)).toBe('');
|
||||
checkFilesExist(
|
||||
`packages/${pkg2}/dist/src/file1.js`,
|
||||
`packages/${pkg2}/dist/src/file1.d.ts`,
|
||||
`packages/${pkg2}/dist/src/nested/file2.js`,
|
||||
`packages/${pkg2}/dist/src/nested/file2.d.ts`,
|
||||
`packages/${pkg2}/dist/src/file3.js`,
|
||||
`packages/${pkg2}/dist/src/file3.d.ts`,
|
||||
`packages/${pkg2}/dist/src/file4.js`,
|
||||
`packages/${pkg2}/dist/src/file4.d.ts`,
|
||||
`packages/${pkg2}/dist/src/main.js`,
|
||||
`packages/${pkg2}/dist/src/main.d.ts`
|
||||
);
|
||||
});
|
||||
|
||||
function createPackage(
|
||||
name: string,
|
||||
options?: {
|
||||
@ -328,6 +478,7 @@ describe('Graph - TS solution setup', () => {
|
||||
main?: string;
|
||||
types?: string;
|
||||
exports?: string | Record<string, any>;
|
||||
imports?: Record<string, any>;
|
||||
};
|
||||
}
|
||||
): void {
|
||||
|
||||
@ -143,9 +143,8 @@ export class TargetProjectLocator {
|
||||
}
|
||||
|
||||
if (this.tsConfig.config) {
|
||||
// TODO(meeroslav): this block is probably obsolete
|
||||
// and existed only because of the incomplete `paths` matching
|
||||
// if import cannot be matched using tsconfig `paths` the compilation would fail anyway
|
||||
// TODO: this can be removed once we rework resolveImportWithRequire below
|
||||
// to properly handle ESM (exports, imports, conditions)
|
||||
const resolvedProject = this.resolveImportWithTypescript(
|
||||
importExpr,
|
||||
filePath
|
||||
@ -369,8 +368,15 @@ export class TargetProjectLocator {
|
||||
filePath: string
|
||||
): string | undefined {
|
||||
let resolvedModule: string;
|
||||
if (this.typescriptResolutionCache.has(normalizedImportExpr)) {
|
||||
resolvedModule = this.typescriptResolutionCache.get(normalizedImportExpr);
|
||||
const projectName = findProjectForPath(filePath, this.projectRootMappings);
|
||||
const cacheScope = projectName
|
||||
? // fall back to the project name if the project root can't be determined
|
||||
this.nodes[projectName]?.data?.root || projectName
|
||||
: // fall back to the file path if the project can't be determined
|
||||
filePath;
|
||||
const cacheKey = `${normalizedImportExpr}__${cacheScope}`;
|
||||
if (this.typescriptResolutionCache.has(cacheKey)) {
|
||||
resolvedModule = this.typescriptResolutionCache.get(cacheKey);
|
||||
} else {
|
||||
resolvedModule = resolveModuleByImport(
|
||||
normalizedImportExpr,
|
||||
@ -378,19 +384,31 @@ export class TargetProjectLocator {
|
||||
this.tsConfig.absolutePath
|
||||
);
|
||||
this.typescriptResolutionCache.set(
|
||||
normalizedImportExpr,
|
||||
cacheKey,
|
||||
resolvedModule ? resolvedModule : null
|
||||
);
|
||||
}
|
||||
|
||||
// TODO: vsavkin temporary workaround. Remove it once we reworking handling of npm packages.
|
||||
if (resolvedModule && resolvedModule.indexOf('node_modules/') === -1) {
|
||||
const resolvedProject = this.findProjectOfResolvedModule(resolvedModule);
|
||||
if (resolvedProject) {
|
||||
return resolvedProject;
|
||||
}
|
||||
if (!resolvedModule) {
|
||||
return;
|
||||
}
|
||||
return;
|
||||
|
||||
const nodeModulesIndex = resolvedModule.lastIndexOf('node_modules/');
|
||||
if (nodeModulesIndex === -1) {
|
||||
const resolvedProject = this.findProjectOfResolvedModule(resolvedModule);
|
||||
return resolvedProject;
|
||||
}
|
||||
|
||||
// strip the node_modules/ prefix from the resolved module path
|
||||
const packagePath = resolvedModule.substring(
|
||||
nodeModulesIndex + 'node_modules/'.length
|
||||
);
|
||||
const externalProject = this.findNpmProjectFromImport(
|
||||
packagePath,
|
||||
filePath
|
||||
);
|
||||
|
||||
return externalProject;
|
||||
}
|
||||
|
||||
private resolveImportWithRequire(
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user