From dddc1b1e6cb067018f39b018f2a5f19b031e60cc Mon Sep 17 00:00:00 2001 From: Jonathan Cammisuli Date: Fri, 3 Jan 2020 13:27:19 -0500 Subject: [PATCH] fix(core): sort node names for module resolution --- .../src/rules/enforce-module-boundaries.ts | 11 ++ .../rules/enforce-module-boundaries.spec.ts | 4 + .../explicit-project-dependencies.ts | 9 +- .../build-dependencies/find-target-project.ts | 25 ---- ...spec.ts => target-project-locator.spec.ts} | 113 +++++++++++++++--- .../target-project-locator.ts | 45 +++++++ .../nxEnforceModuleBoundariesRule.spec.ts | 4 +- .../tslint/nxEnforceModuleBoundariesRule.ts | 18 ++- .../workspace/src/utils/runtime-lint-utils.ts | 8 +- packages/workspace/src/utils/typescript.ts | 2 +- 10 files changed, 181 insertions(+), 58 deletions(-) delete mode 100644 packages/workspace/src/core/project-graph/build-dependencies/find-target-project.ts rename packages/workspace/src/core/project-graph/build-dependencies/{find-target-project.spec.ts => target-project-locator.spec.ts} (53%) create mode 100644 packages/workspace/src/core/project-graph/build-dependencies/target-project-locator.ts diff --git a/packages/eslint-plugin-nx/src/rules/enforce-module-boundaries.ts b/packages/eslint-plugin-nx/src/rules/enforce-module-boundaries.ts index c6db35c408..89e8f4928f 100644 --- a/packages/eslint-plugin-nx/src/rules/enforce-module-boundaries.ts +++ b/packages/eslint-plugin-nx/src/rules/enforce-module-boundaries.ts @@ -25,6 +25,7 @@ import { readNxJson, readWorkspaceJson } from '@nrwl/workspace/src/core/file-utils'; +import { TargetProjectLocator } from '@nrwl/workspace/src/core/project-graph/build-dependencies/target-project-locator'; type Options = [ { @@ -100,6 +101,15 @@ export default createESLintRule({ } const npmScope = (global as any).npmScope; const projectGraph = (global as any).projectGraph as ProjectGraph; + + if (!(global as any).targetProjectLocator) { + (global as any).targetProjectLocator = new TargetProjectLocator( + projectGraph.nodes + ); + } + const targetProjectLocator = (global as any) + .targetProjectLocator as TargetProjectLocator; + return { ImportDeclaration(node: TSESTree.ImportDeclaration) { const imp = (node.source as TSESTree.Literal).value as string; @@ -141,6 +151,7 @@ export default createESLintRule({ // findProjectUsingImport to take care of same prefix const targetProject = findProjectUsingImport( projectGraph, + targetProjectLocator, sourceFilePath, imp, npmScope diff --git a/packages/eslint-plugin-nx/tests/rules/enforce-module-boundaries.spec.ts b/packages/eslint-plugin-nx/tests/rules/enforce-module-boundaries.spec.ts index fb3b3c0082..521223eadd 100644 --- a/packages/eslint-plugin-nx/tests/rules/enforce-module-boundaries.spec.ts +++ b/packages/eslint-plugin-nx/tests/rules/enforce-module-boundaries.spec.ts @@ -10,6 +10,7 @@ import { extname } from 'path'; import enforceModuleBoundaries, { RULE_NAME as enforceModuleBoundariesRuleName } from '../../src/rules/enforce-module-boundaries'; +import { TargetProjectLocator } from '@nrwl/workspace/src/core/project-graph/build-dependencies/target-project-locator'; jest.mock('fs', () => require('memfs').fs); jest.mock('@nrwl/workspace/src/utils/app-root', () => ({ appRootPath: '/root' @@ -824,6 +825,9 @@ function runRule( (global as any).projectPath = `${process.cwd()}/proj`; (global as any).npmScope = 'mycompany'; (global as any).projectGraph = projectGraph; + (global as any).targetProjectLocator = new TargetProjectLocator( + projectGraph.nodes + ); const config = { ...baseConfig, diff --git a/packages/workspace/src/core/project-graph/build-dependencies/explicit-project-dependencies.ts b/packages/workspace/src/core/project-graph/build-dependencies/explicit-project-dependencies.ts index e3c2385259..ea9bf9e28c 100644 --- a/packages/workspace/src/core/project-graph/build-dependencies/explicit-project-dependencies.ts +++ b/packages/workspace/src/core/project-graph/build-dependencies/explicit-project-dependencies.ts @@ -5,7 +5,7 @@ import { ProjectGraphNodeRecords } from '../project-graph-models'; import { TypeScriptImportLocator } from './typescript-import-locator'; -import { findTargetProjectWithImport } from './find-target-project'; +import { TargetProjectLocator } from './target-project-locator'; export function buildExplicitTypeScriptDependencies( ctx: ProjectGraphContext, @@ -14,17 +14,16 @@ export function buildExplicitTypeScriptDependencies( fileRead: (s: string) => string ) { const importLocator = new TypeScriptImportLocator(fileRead); - + const targetProjectLocator = new TargetProjectLocator(nodes); Object.keys(ctx.fileMap).forEach(source => { Object.values(ctx.fileMap[source]).forEach(f => { importLocator.fromFile( f.file, (importExpr: string, filePath: string, type: DependencyType) => { - const target = findTargetProjectWithImport( + const target = targetProjectLocator.findProjectWithImport( importExpr, f.file, - ctx.nxJson.npmScope, - nodes + ctx.nxJson.npmScope ); if (source && target) { addDependency(type, source, target); diff --git a/packages/workspace/src/core/project-graph/build-dependencies/find-target-project.ts b/packages/workspace/src/core/project-graph/build-dependencies/find-target-project.ts deleted file mode 100644 index 91f7509c46..0000000000 --- a/packages/workspace/src/core/project-graph/build-dependencies/find-target-project.ts +++ /dev/null @@ -1,25 +0,0 @@ -import { resolveModuleByImport } from '../../../utils/typescript'; -import { normalizedProjectRoot } from '../../file-utils'; -import { ProjectGraphNodeRecords } from '../project-graph-models'; - -export function findTargetProjectWithImport( - importExpr: string, - filePath: string, - npmScope: string, - nodes: ProjectGraphNodeRecords -) { - const normalizedImportExpr = importExpr.split('#')[0]; - - const resolvedModule = resolveModuleByImport(normalizedImportExpr, filePath); - - return Object.keys(nodes).find(projectName => { - const p = nodes[projectName]; - if (resolvedModule && resolvedModule.includes(p.data.root)) { - return true; - } else { - const normalizedRoot = normalizedProjectRoot(p); - const projectImport = `@${npmScope}/${normalizedRoot}`; - return normalizedImportExpr.startsWith(projectImport); - } - }); -} diff --git a/packages/workspace/src/core/project-graph/build-dependencies/find-target-project.spec.ts b/packages/workspace/src/core/project-graph/build-dependencies/target-project-locator.spec.ts similarity index 53% rename from packages/workspace/src/core/project-graph/build-dependencies/find-target-project.spec.ts rename to packages/workspace/src/core/project-graph/build-dependencies/target-project-locator.spec.ts index 3e1778cc3c..0eef68f47b 100644 --- a/packages/workspace/src/core/project-graph/build-dependencies/find-target-project.spec.ts +++ b/packages/workspace/src/core/project-graph/build-dependencies/target-project-locator.spec.ts @@ -1,7 +1,7 @@ import { fs, vol } from 'memfs'; import { join } from 'path'; -import { ProjectGraphContext, ProjectGraphNode } from '../../project-graph'; -import { findTargetProjectWithImport } from './find-target-project'; +import { ProjectGraphContext, ProjectGraphNode } from '../project-graph-models'; +import { TargetProjectLocator } from './target-project-locator'; jest.mock('../../../utils/app-root', () => ({ appRootPath: '/root' @@ -12,6 +12,7 @@ describe('findTargetProjectWithImport', () => { let ctx: ProjectGraphContext; let projects: Record; let fsJson; + let targetProjectLocator: TargetProjectLocator; beforeEach(() => { const workspaceJson = { projects: { @@ -30,7 +31,10 @@ describe('findTargetProjectWithImport', () => { paths: { '@proj/proj1': ['libs/proj1/index.ts'], '@proj/my-second-proj': ['libs/proj2/index.ts'], - '@proj/project-3': ['libs/proj3/index.ts'] + '@proj/project-3': ['libs/proj3/index.ts'], + '@proj/proj123': ['libs/proj123/index.ts'], + '@proj/proj1234': ['libs/proj1234/index.ts'], + '@proj/proj1234-child': ['libs/proj1234-child/index.ts'] } } }; @@ -44,7 +48,10 @@ describe('findTargetProjectWithImport', () => { `, './libs/proj2/index.ts': `export const a = 2;`, './libs/proj3/index.ts': `export const a = 3;`, - './libs/proj4/index.ts': `export const a = 4;` + './libs/proj4/index.ts': `export const a = 4;`, + './libs/proj123/index.ts': 'export const a = 5', + './libs/proj1234/index.ts': 'export const a = 6', + './libs/proj1234-child/index.ts': 'export const a = 7' }; vol.fromJSON(fsJson, '/root'); @@ -79,15 +86,38 @@ describe('findTargetProjectWithImport', () => { mtime: 0, ext: '.ts' } + ], + proj123: [ + { + file: 'libs/proj123/index.ts', + mtime: 0, + ext: '.ts' + } + ], + proj1234: [ + { + file: 'libs/proj1234/index.ts', + mtime: 0, + ext: '.ts' + } + ], + 'proj1234-child': [ + { + file: 'libs/proj1234-child/index.ts', + mtime: 0, + ext: '.ts' + } ] } }; + + // these projects should be in a random order projects = { - proj1: { - name: 'proj1', + proj4: { + name: 'proj4', type: 'lib', data: { - root: 'libs/proj1', + root: 'libs/proj4', files: [] } }, @@ -99,6 +129,22 @@ describe('findTargetProjectWithImport', () => { files: [] } }, + proj1: { + name: 'proj1', + type: 'lib', + data: { + root: 'libs/proj1', + files: [] + } + }, + proj1234: { + name: 'proj1234', + type: 'lib', + data: { + root: 'libs/proj1234', + files: [] + } + }, proj3: { name: 'proj3', type: 'lib', @@ -107,41 +153,70 @@ describe('findTargetProjectWithImport', () => { files: [] } }, - proj4: { - name: 'proj4', + proj123: { + name: 'proj123', type: 'lib', data: { - root: 'libs/proj4', + root: 'libs/proj123', + files: [] + } + }, + 'proj1234-child': { + name: 'proj1234-child', + type: 'lib', + data: { + root: 'libs/proj1234-child', files: [] } } }; + + targetProjectLocator = new TargetProjectLocator(projects); }); it('should be able to resolve a module by using tsConfig paths', () => { - const proj2 = findTargetProjectWithImport( + const proj2 = targetProjectLocator.findProjectWithImport( '@proj/my-second-proj', 'libs/proj1/index.ts', - ctx.nxJson.npmScope, - projects + ctx.nxJson.npmScope ); - const proj3 = findTargetProjectWithImport( + const proj3 = targetProjectLocator.findProjectWithImport( '@proj/project-3', 'libs/proj1/index.ts', - ctx.nxJson.npmScope, - projects + ctx.nxJson.npmScope ); expect(proj2).toEqual('proj2'); expect(proj3).toEqual('proj3'); }); it('should be able to resolve a module using a normalized path', () => { - const proj4 = findTargetProjectWithImport( + const proj4 = targetProjectLocator.findProjectWithImport( '@proj/proj4', 'libs/proj1/index.ts', - ctx.nxJson.npmScope, - projects + ctx.nxJson.npmScope ); expect(proj4).toEqual('proj4'); }); + it('should be able to resolve paths that have similar names', () => { + const proj = targetProjectLocator.findProjectWithImport( + '@proj/proj123', + '', + ctx.nxJson.npmScope + ); + expect(proj).toEqual('proj123'); + + const childProj = targetProjectLocator.findProjectWithImport( + '@proj/proj1234-child', + '', + ctx.nxJson.npmScope + ); + expect(childProj).toEqual('proj1234-child'); + + const parentProj = targetProjectLocator.findProjectWithImport( + '@proj/proj1234', + '', + ctx.nxJson.npmScope + ); + expect(parentProj).toEqual('proj1234'); + }); }); diff --git a/packages/workspace/src/core/project-graph/build-dependencies/target-project-locator.ts b/packages/workspace/src/core/project-graph/build-dependencies/target-project-locator.ts new file mode 100644 index 0000000000..4f9094f694 --- /dev/null +++ b/packages/workspace/src/core/project-graph/build-dependencies/target-project-locator.ts @@ -0,0 +1,45 @@ +import { resolveModuleByImport } from '../../../utils/typescript'; +import { normalizedProjectRoot } from '../../file-utils'; +import { ProjectGraphNodeRecords } from '../project-graph-models'; + +export class TargetProjectLocator { + private sortedNodeNames = []; + + constructor(private nodes: ProjectGraphNodeRecords) { + this.sortedNodeNames = Object.keys(nodes).sort((a, b) => { + if (!nodes[a].data.root) return -1; + if (!nodes[b].data.root) return -1; + return nodes[a].data.root.length > nodes[b].data.root.length ? -1 : 1; + }); + } + + findProjectWithImport( + importExpr: string, + filePath: string, + npmScope: string + ) { + const normalizedImportExpr = importExpr.split('#')[0]; + + const resolvedModule = resolveModuleByImport( + normalizedImportExpr, + filePath + ); + + return this.sortedNodeNames.find(projectName => { + const p = this.nodes[projectName]; + + // If there is no root in the data object, it's not a nx project (ie. npm module) + if (!p.data.root) { + return false; + } + + if (resolvedModule && resolvedModule.startsWith(p.data.root)) { + return true; + } else { + const normalizedRoot = normalizedProjectRoot(p); + const projectImport = `@${npmScope}/${normalizedRoot}`; + return normalizedImportExpr.startsWith(projectImport); + } + }); + } +} diff --git a/packages/workspace/src/tslint/nxEnforceModuleBoundariesRule.spec.ts b/packages/workspace/src/tslint/nxEnforceModuleBoundariesRule.spec.ts index a52e16e35f..7306f0dc0d 100644 --- a/packages/workspace/src/tslint/nxEnforceModuleBoundariesRule.spec.ts +++ b/packages/workspace/src/tslint/nxEnforceModuleBoundariesRule.spec.ts @@ -8,6 +8,7 @@ import { ProjectType } from '../core/project-graph'; import { Rule } from './nxEnforceModuleBoundariesRule'; +import { TargetProjectLocator } from '../core/project-graph/build-dependencies/target-project-locator'; jest.mock('fs', () => require('memfs').fs); jest.mock('../utils/app-root', () => ({ appRootPath: '/root' })); @@ -821,7 +822,8 @@ function runRule( options, `${process.cwd()}/proj`, 'mycompany', - projectGraph + projectGraph, + new TargetProjectLocator(projectGraph.nodes) ); return rule.apply(sourceFile); } diff --git a/packages/workspace/src/tslint/nxEnforceModuleBoundariesRule.ts b/packages/workspace/src/tslint/nxEnforceModuleBoundariesRule.ts index abcf9d8e1c..98e0f28e25 100644 --- a/packages/workspace/src/tslint/nxEnforceModuleBoundariesRule.ts +++ b/packages/workspace/src/tslint/nxEnforceModuleBoundariesRule.ts @@ -28,13 +28,15 @@ import { readNxJson, readWorkspaceJson } from '@nrwl/workspace/src/core/file-utils'; +import { TargetProjectLocator } from '../core/project-graph/build-dependencies/target-project-locator'; export class Rule extends Lint.Rules.AbstractRule { constructor( options: IOptions, private readonly projectPath?: string, private readonly npmScope?: string, - private readonly projectGraph?: ProjectGraph + private readonly projectGraph?: ProjectGraph, + private readonly targetProjectLocator?: TargetProjectLocator ) { super(options); @@ -51,6 +53,13 @@ export class Rule extends Lint.Rules.AbstractRule { } this.npmScope = (global as any).npmScope; this.projectGraph = (global as any).projectGraph; + + if (!(global as any).targetProjectLocator) { + (global as any).targetProjectLocator = new TargetProjectLocator( + this.projectGraph.nodes + ); + } + this.targetProjectLocator = (global as any).targetProjectLocator; } } @@ -61,7 +70,8 @@ export class Rule extends Lint.Rules.AbstractRule { this.getOptions(), this.projectPath, this.npmScope, - this.projectGraph + this.projectGraph, + this.targetProjectLocator ) ); } @@ -76,7 +86,8 @@ class EnforceModuleBoundariesWalker extends Lint.RuleWalker { options: IOptions, private readonly projectPath: string, private readonly npmScope: string, - private readonly projectGraph: ProjectGraph + private readonly projectGraph: ProjectGraph, + private readonly targetProjectLocator: TargetProjectLocator ) { super(sourceFile, options); @@ -132,6 +143,7 @@ class EnforceModuleBoundariesWalker extends Lint.RuleWalker { // findProjectUsingImport to take care of same prefix const targetProject = findProjectUsingImport( this.projectGraph, + this.targetProjectLocator, filePath, imp, this.npmScope diff --git a/packages/workspace/src/utils/runtime-lint-utils.ts b/packages/workspace/src/utils/runtime-lint-utils.ts index 0e8d9299ec..8ed4bc8977 100644 --- a/packages/workspace/src/utils/runtime-lint-utils.ts +++ b/packages/workspace/src/utils/runtime-lint-utils.ts @@ -7,7 +7,7 @@ import { ProjectGraphDependency, ProjectGraphNode } from '../core/project-graph'; -import { findTargetProjectWithImport } from '../core/project-graph/build-dependencies/find-target-project'; +import { TargetProjectLocator } from '../core/project-graph/build-dependencies/target-project-locator'; export type Deps = { [projectName: string]: ProjectGraphDependency[] }; export type DepConstraint = { @@ -134,15 +134,15 @@ export function isAbsoluteImportIntoAnotherProject(imp: string) { export function findProjectUsingImport( projectGraph: ProjectGraph, + targetProjectLocator: TargetProjectLocator, filePath: string, imp: string, npmScope: string ) { - const target = findTargetProjectWithImport( + const target = targetProjectLocator.findProjectWithImport( imp, filePath, - npmScope, - projectGraph.nodes + npmScope ); return projectGraph.nodes[target]; } diff --git a/packages/workspace/src/utils/typescript.ts b/packages/workspace/src/utils/typescript.ts index c963ddf950..a28d192e17 100644 --- a/packages/workspace/src/utils/typescript.ts +++ b/packages/workspace/src/utils/typescript.ts @@ -38,7 +38,7 @@ export function resolveModuleByImport(importExpr: string, filePath: string) { if (!resolvedModule) { return; } else { - return resolvedModule.resolvedFileName.replace(appRootPath, ''); + return resolvedModule.resolvedFileName.replace(`${appRootPath}/`, ''); } }