diff --git a/e2e/linter/src/linter.test.ts b/e2e/linter/src/linter.test.ts index 607b9c0068a139..e47e1183e7f632 100644 --- a/e2e/linter/src/linter.test.ts +++ b/e2e/linter/src/linter.test.ts @@ -1,6 +1,7 @@ import * as path from 'path'; import { checkFilesExist, + createFile, newProject, readFile, readJson, @@ -9,6 +10,7 @@ import { updateFile, } from '@nrwl/e2e/utils'; import * as ts from 'typescript'; +import { names } from '@nrwl/devkit'; describe('Linter', () => { describe('linting errors', () => { @@ -191,6 +193,249 @@ describe('Linter', () => { expect(lintOutput).toContain(knownLintErrorMessage); }, 1000000); }); + + it('lint plugin should ensure module boundaries', () => { + const proj = newProject(); + const myapp = uniq('myapp'); + const myapp2 = uniq('myapp2'); + const mylib = uniq('mylib'); + const lazylib = uniq('lazylib'); + const invalidtaglib = uniq('invalidtaglib'); + const validtaglib = uniq('validtaglib'); + + runCLI(`generate @nrwl/angular:app ${myapp} --tags=validtag`); + runCLI(`generate @nrwl/angular:app ${myapp2}`); + runCLI(`generate @nrwl/angular:lib ${mylib}`); + runCLI(`generate @nrwl/angular:lib ${lazylib}`); + runCLI(`generate @nrwl/angular:lib ${invalidtaglib} --tags=invalidtag`); + runCLI(`generate @nrwl/angular:lib ${validtaglib} --tags=validtag`); + + const eslint = readJson('.eslintrc.json'); + eslint.overrides[0].rules[ + '@nrwl/nx/enforce-module-boundaries' + ][1].depConstraints = [ + { sourceTag: 'validtag', onlyDependOnLibsWithTags: ['validtag'] }, + ...eslint.overrides[0].rules['@nrwl/nx/enforce-module-boundaries'][1] + .depConstraints, + ]; + updateFile('.eslintrc.json', JSON.stringify(eslint, null, 2)); + + const tsConfig = readJson('tsconfig.base.json'); + + /** + * apps do not add themselves to the tsconfig file. + * + * Let's add it so that we can trigger the lint failure + */ + tsConfig.compilerOptions.paths[`@${proj}/${myapp2}`] = [ + `apps/${myapp2}/src/main.ts`, + ]; + + tsConfig.compilerOptions.paths[`@secondScope/${lazylib}`] = + tsConfig.compilerOptions.paths[`@${proj}/${lazylib}`]; + delete tsConfig.compilerOptions.paths[`@${proj}/${lazylib}`]; + updateFile('tsconfig.base.json', JSON.stringify(tsConfig, null, 2)); + + updateFile( + `apps/${myapp}/src/main.ts`, + ` + import '../../../libs/${mylib}'; + import '@secondScope/${lazylib}'; + import '@${proj}/${myapp2}'; + import '@${proj}/${invalidtaglib}'; + import '@${proj}/${validtaglib}'; + + const s = {loadChildren: '@${proj}/${lazylib}'}; + ` + ); + + const out = runCLI(`lint ${myapp}`, { silenceError: true }); + expect(out).toContain( + 'Projects cannot be imported by a relative or absolute path, and must begin with a npm scope' + ); + expect(out).toContain('Imports of apps are forbidden'); + expect(out).toContain( + 'A project tagged with "validtag" can only depend on libs tagged with "validtag"' + ); + }, 1000000); + + describe('workspace boundary rules', () => { + const libA = uniq('tslib-a'); + const libB = uniq('tslib-b'); + const libC = uniq('tslib-c'); + let projScope; + + beforeAll(() => { + projScope = newProject(); + runCLI(`generate @nrwl/workspace:lib ${libA}`); + runCLI(`generate @nrwl/workspace:lib ${libB}`); + runCLI(`generate @nrwl/workspace:lib ${libC}`); + + /** + * create tslib-a structure + */ + createFile( + `libs/${libA}/src/lib/tslib-a.ts`, + ` + export function libASayHello(): string { + return 'Hi from tslib-a'; + } + ` + ); + + createFile( + `libs/${libA}/src/lib/some-non-exported-function.ts`, + ` + export function someNonPublicLibFunction() { + return 'this function is exported, but not via the libs barrel file'; + } + + export function someSelectivelyExportedFn() { + return 'this fn is exported selectively in the barrel file'; + } + ` + ); + + createFile( + `libs/${libA}/src/index.ts`, + ` + export * from './lib/tslib-a'; + + export { someSelectivelyExportedFn } from './lib/some-non-exported-function'; + ` + ); + + /** + * create tslib-b structure + */ + createFile( + `libs/${libB}/src/index.ts`, + ` + export * from './lib/tslib-b'; + ` + ); + + createFile( + `libs/${libB}/src/lib/tslib-b.ts`, + ` + import { libASayHello } from '../../../${libA}/src/lib/tslib-a'; + // import { someNonPublicLibFunction } from '../../../${libA}/src/lib/some-non-exported-function'; + import { someSelectivelyExportedFn } from '../../../${libA}/src/lib/some-non-exported-function'; + + export function tslibB(): string { + // someNonPublicLibFunction(); + someSelectivelyExportedFn(); + libASayHello(); + return 'hi there'; + } + ` + ); + + /** + * create tslib-c structure + */ + + createFile( + `libs/${libC}/src/index.ts`, + ` + export * from './lib/tslib-c'; + export * from './lib/constant'; + + ` + ); + + createFile( + `libs/${libC}/src/lib/constant.ts`, + ` + export const SOME_CONSTANT = 'some constant value'; + export const someFunc1 = () => 'hi'; + export function someFunc2() { + return 'hi2'; + } + ` + ); + + createFile( + `libs/${libC}/src/lib/tslib-c-another.ts`, + ` +import { tslibC, SOME_CONSTANT, someFunc1, someFunc2 } from '@${projScope}/${libC}'; + +export function someStuff() { + someFunc1(); + someFunc2(); + tslibC(); + console.log(SOME_CONSTANT); + return 'hi'; +} + + ` + ); + + createFile( + `libs/${libC}/src/lib/tslib-c.ts`, + ` +import { someFunc1, someFunc2, SOME_CONSTANT } from '@${projScope}/${libC}'; + +export function tslibC(): string { + someFunc1(); + someFunc2(); + console.log(SOME_CONSTANT); + return 'tslib-c'; +} + + ` + ); + }); + + it('should fix noSelfCircularDependencies', () => { + const stdout = runCLI(`lint ${libC}`, { + silenceError: true, + }); + expect(stdout).toContain( + 'Projects should use relative imports to import from other files within the same project' + ); + + // fix them + const fixedStout = runCLI(`lint ${libC} --fix`, { + silenceError: true, + }); + expect(fixedStout).toContain('Successfully ran target lint for project'); + + const fileContent = readFile(`libs/${libC}/src/lib/tslib-c-another.ts`); + expect(fileContent).toContain(` + import { tslibC } from './tslib-c'; + import { SOME_CONSTANT, someFunc1, someFunc2 } from './constant'; + `); + + const fileContentTslibC = readFile(`libs/${libC}/src/lib/tslib-c.ts`); + expect(fileContentTslibC).toContain(` + import { someFunc1, someFunc2, SOME_CONSTANT } from './constant'; + `); + }); + + it('should fix noRelativeOrAbsoluteImportsAcrossLibraries', () => { + const stdout = runCLI(`lint ${libB}`, { + silenceError: true, + }); + expect(stdout).toContain( + 'Projects cannot be imported by a relative or absolute path, and must begin with a npm scope' + ); + + // fix them + const fixedStout = runCLI(`lint ${libB} --fix`, { + silenceError: true, + }); + expect(fixedStout).toContain('Successfully ran target lint for project'); + + const fileContent = readFile(`libs/${libB}/src/lib/tslib-b.ts`); + expect(fileContent).toContain( + `import { libASayHello } from '@${projScope}/${libA}';` + ); + expect(fileContent).toContain( + `import { someSelectivelyExportedFn } from '@${projScope}/${libA}';` + ); + }); + }); }); /** 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 d1723c1222cc23..35795a3ed90148 100644 --- a/packages/eslint-plugin-nx/src/rules/enforce-module-boundaries.ts +++ b/packages/eslint-plugin-nx/src/rules/enforce-module-boundaries.ts @@ -15,6 +15,7 @@ import { MappedProjectGraph, hasBannedImport, isDirectDependency, + getTargetProjectBasedOnRelativeImport, isTerminalRun, } from '@nrwl/workspace/src/utils/runtime-lint-utils'; import { @@ -22,7 +23,7 @@ import { TSESTree, } from '@typescript-eslint/experimental-utils'; import { createESLintRule } from '../utils/create-eslint-rule'; -import { normalizePath } from '@nrwl/devkit'; +import { joinPathFragments, normalizePath } from '@nrwl/devkit'; import { ProjectType, readCachedProjectGraph, @@ -36,6 +37,15 @@ import { import { isRelativePath } from '@nrwl/workspace/src/utilities/fileutils'; import { isSecondaryEntrypoint as isAngularSecondaryEntrypoint } from '../utils/angular'; import * as chalk from 'chalk'; +import { existsSync, readFileSync } from 'fs'; +import { findNodes } from '@nrwl/workspace/src/utilities/typescript'; +import ts = require('typescript'); +import { basename, dirname, join, relative, resolve } from 'path'; +import { + getBarrelEntryPointByImportScope, + getBarrelEntryPointProjectNode, + getRelativeImportPath, +} from '../utils/ast-utils'; type Options = [ { @@ -228,6 +238,95 @@ export default createESLintRule({ data: { npmScope, }, + fix(fixer) { + const targetImportProject = getTargetProjectBasedOnRelativeImport( + imp, + projectPath, + projectGraph, + sourceFilePath + ); + + if (targetImportProject) { + const indexTsPaths = + getBarrelEntryPointProjectNode(targetImportProject); + + if (indexTsPaths && indexTsPaths.length > 0) { + const imports = ((node as any).specifiers || []).map( + (specifier) => specifier.imported.name + ); + + // ignore non-named imports for now, like "import '../../some-other-lib/src'" + if (imports.length === 0) { + return; + } + + // process each potential entry point and try to find the imports + const importsToRemap = []; + + for (const entryPointPath of indexTsPaths) { + for (const importMember of imports) { + const importPath = getRelativeImportPath( + importMember, + entryPointPath.path, + sourceProject.data.sourceRoot + ); + + if (importPath) { + importsToRemap.push({ + member: importMember, + importScope: entryPointPath.importScope, + }); + } else { + // we cannot remap, so leave it as is + importsToRemap.push({ + member: importMember, + importScope: imp, + }); + } + } + } + + // group imports together, like having "import { Foo, Bar } from './foo'" + // instead of individual ones + const importsToRemapGrouped = importsToRemap.reduce( + (acc, curr) => { + const existing = acc.find( + (i) => + i.importScope === curr.importScope && + i.member !== curr.member + ); + if (existing) { + if (existing.member) { + existing.member += `, ${curr.member}`; + } + } else { + acc.push({ + importScope: curr.importScope, + member: curr.member, + }); + } + return acc; + }, + [] + ); + + // create the new import expressions + const adjustedRelativeImports = importsToRemapGrouped + .map( + (entry) => + `import { ${entry.member} } from '${entry.importScope}';` + ) + .join('\n'); + + if (adjustedRelativeImports !== '') { + return fixer.replaceTextRange( + node.range, + adjustedRelativeImports + ); + } + } + } + }, }); return; } @@ -261,6 +360,91 @@ export default createESLintRule({ data: { imp, }, + fix(fixer) { + // imp is equal to @myorg/someproject + const indexTsPaths = getBarrelEntryPointByImportScope(imp); + if (indexTsPaths && indexTsPaths.length > 0) { + // imported JS functions to remap + const imports = ( + (node.source.parent as any).specifiers || [] + ).map((specifier) => specifier.imported.name); + + // ignore non-named imports for now, like "import '../../some-other-lib/src'" + if (imports.length === 0) { + return; + } + + // process each potential entry point and try to find the imports + const importsToRemap = []; + + for (const entryPointPath of indexTsPaths) { + for (const importMember of imports) { + const importPath = getRelativeImportPath( + importMember, + entryPointPath, + sourceProject.data.sourceRoot + ); + if (importPath) { + // resolve the import path + let importPathResolved = relative( + dirname(context.getFilename()), + dirname(importPath) + ); + + // if the string is empty, it's the current file + importPathResolved = + importPathResolved === '' + ? `./${basename(importPath)}` + : joinPathFragments( + importPathResolved, + basename(importPath) + ); + + importsToRemap.push({ + member: importMember, + relativePath: importPathResolved.replace('.ts', ''), + }); + } + } + } + + // group imports from the same relative path + const importsToRemapGrouped = importsToRemap.reduce( + (acc, curr) => { + const existing = acc.find( + (i) => + i.relativePath === curr.relativePath && + i.member !== curr.member + ); + if (existing) { + if (existing.member) { + existing.member += `, ${curr.member}`; + } + } else { + acc.push({ + relativePath: curr.relativePath, + member: curr.member, + }); + } + return acc; + }, + [] + ); + + const adjustedRelativeImports = importsToRemapGrouped + .map( + (entry) => + `import { ${entry.member} } from '${entry.relativePath}';` + ) + .join('\n'); + if (adjustedRelativeImports !== '') { + return fixer.replaceTextRange( + node.range, + adjustedRelativeImports + ); + } + } + }, }); } return; diff --git a/packages/eslint-plugin-nx/src/utils/ast-utils.ts b/packages/eslint-plugin-nx/src/utils/ast-utils.ts new file mode 100644 index 00000000000000..af2128df7de242 --- /dev/null +++ b/packages/eslint-plugin-nx/src/utils/ast-utils.ts @@ -0,0 +1,235 @@ +import { joinPathFragments } from '@nrwl/devkit'; +import { findNodes } from '@nrwl/workspace/src/utilities/typescript'; +import { MappedProjectGraphNode } from '@nrwl/workspace/src/utils/runtime-lint-utils'; +import { existsSync, readFileSync } from 'fs'; +import { dirname } from 'path'; +import ts = require('typescript'); +import { logger } from '@nrwl/devkit'; +import { appRootPath } from '@nrwl/tao/src/utils/app-root'; + +function tryReadBaseJson() { + try { + return JSON.parse( + readFileSync( + joinPathFragments(appRootPath, 'tsconfig.base.json') + ).toString('utf-8') + ); + } catch (e) { + logger.warn(`Error reading tsconfig.base.json: \n${JSON.stringify(e)}`); + return null; + } +} + +/** + * + * @param importScope like `@myorg/somelib` + * @returns + */ +export function getBarrelEntryPointByImportScope( + importScope: string +): string[] | null { + const tsConfigBase = tryReadBaseJson(); + + if (tsConfigBase?.compilerOptions?.paths) { + const entryPoints = tsConfigBase.compilerOptions.paths[importScope]; + if (entryPoints) { + return entryPoints; + } + } + return null; +} + +export function getBarrelEntryPointProjectNode( + importScope: MappedProjectGraphNode +): { path: string; importScope: string }[] | null { + const tsConfigBase = tryReadBaseJson(); + + if (tsConfigBase?.compilerOptions?.paths) { + const potentialEntryPoints = Object.keys(tsConfigBase.compilerOptions.paths) + .filter((entry) => { + const sourceFolderPaths = tsConfigBase.compilerOptions.paths[entry]; + return sourceFolderPaths.some((sourceFolderPath) => { + return sourceFolderPath.includes(importScope.data.root); + }); + }) + .map((entry) => + tsConfigBase.compilerOptions.paths[entry].map((x) => ({ + path: x, + importScope: entry, + })) + ); + + return potentialEntryPoints.flat(); + } + + return null; +} + +function hasMemberExport(exportedMember, filePath) { + const fileContent = readFileSync(filePath, 'utf8'); + + // use the TypeScript AST to find the path to the file where exportedMember is defined + const sourceFile = ts.createSourceFile( + filePath, + fileContent, + ts.ScriptTarget.Latest, + true + ); + + // search whether there is already an export with our node + return ( + findNodes(sourceFile, ts.SyntaxKind.Identifier).filter( + (identifier: any) => identifier.text === exportedMember + ).length > 0 + ); +} + +export function getRelativeImportPath(exportedMember, filePath, basePath) { + const fileContent = readFileSync(filePath, 'utf8'); + + // use the TypeScript AST to find the path to the file where exportedMember is defined + const sourceFile = ts.createSourceFile( + filePath, + fileContent, + ts.ScriptTarget.Latest, + true + ); + + // Search in the current file whether there's an export already! + const memberNodes = findNodes(sourceFile, ts.SyntaxKind.Identifier).filter( + (identifier: any) => identifier.text === exportedMember + ); + + let hasExport = false; + for (const memberNode of memberNodes || []) { + if (memberNode) { + // recursively navigate upwards to find the ExportKey modifier + let parent = memberNode; + do { + parent = parent.parent; + if (parent) { + // if we are inside a parameter list or decorator or param assignment + // then this is not what we're searching for, so break :) + if ( + parent.kind === ts.SyntaxKind.Parameter || + parent.kind === ts.SyntaxKind.PropertyAccessExpression || + parent.kind === ts.SyntaxKind.TypeReference || + parent.kind === ts.SyntaxKind.HeritageClause || + parent.kind === ts.SyntaxKind.Decorator + ) { + hasExport = false; + break; + } + + // if our identifier is within an ExportDeclaration but is not just + // a re-export of some other module, we're good + if ( + parent.kind === ts.SyntaxKind.ExportDeclaration && + !(parent as any).moduleSpecifier + ) { + hasExport = true; + break; + } + + if ( + parent.modifiers && + parent.modifiers.find( + (modifier) => modifier.kind === ts.SyntaxKind.ExportKeyword + ) + ) { + /** + * if we get to a function export declaration we need to verify whether the + * exported function is actually the member we are searching for. Otherwise + * we might end up finding a function that just uses our searched identifier + * internally. + * + * Example: assume we try to find a constant member: `export const SOME_CONSTANT = 'bla'` + * + * Then we might end up in a file that uses it like + * + * import { SOME_CONSTANT } from '@myorg/samelib' + * + * export function someFunction() { + * return `Hi, ${SOME_CONSTANT}` + * } + * + * We want to avoid accidentally picking the someFunction export since we're searching upwards + * starting from `SOME_CONSTANT` identifier usages. + */ + if (parent.kind === ts.SyntaxKind.FunctionDeclaration) { + const parentName = (parent as any)?.name?.text; + if (parentName === exportedMember) { + hasExport = true; + break; + } + } else { + hasExport = true; + break; + } + } + } + } while (!!parent); + } + + if (hasExport) { + break; + } + } + + if (hasExport) { + // we found the file, now grab the path + return filePath; + } + + // if we didn't find an export, let's try to follow + // all export declarations and see whether any of those + // exports the node we're searching for + const exportDeclarations = findNodes( + sourceFile, + ts.SyntaxKind.ExportDeclaration + ) as ts.ExportDeclaration[]; + for (const exportDeclaration of exportDeclarations) { + if ((exportDeclaration as any).moduleSpecifier) { + // verify whether the export declaration we're looking at is a named export + // cause in that case we need to check whether our searched member is + // part of the exports + if ( + exportDeclaration.exportClause && + findNodes(exportDeclaration, ts.SyntaxKind.Identifier).filter( + (identifier: any) => identifier.text === exportedMember + ).length === 0 + ) { + continue; + } + + const modulePath = (exportDeclaration as any).moduleSpecifier.text; + + let moduleFilePath = joinPathFragments( + './', + dirname(filePath), + `${modulePath}.ts` + ); + if (!existsSync(moduleFilePath)) { + // might be a index.ts + moduleFilePath = joinPathFragments( + './', + dirname(filePath), + `${modulePath}/index.ts` + ); + } + + if (hasMemberExport(exportedMember, moduleFilePath)) { + const foundFilePath = getRelativeImportPath( + exportedMember, + moduleFilePath, + basePath + ); + if (foundFilePath) { + return foundFilePath; + } + } + } + } + + return null; +} diff --git a/packages/workspace/src/utils/runtime-lint-utils.ts b/packages/workspace/src/utils/runtime-lint-utils.ts index 2c5c66f719b8d8..9b46f5c7c41eda 100644 --- a/packages/workspace/src/utils/runtime-lint-utils.ts +++ b/packages/workspace/src/utils/runtime-lint-utils.ts @@ -70,20 +70,34 @@ export function isRelative(s: string) { return s.startsWith('.'); } -export function isRelativeImportIntoAnotherProject( +export function getTargetProjectBasedOnRelativeImport( imp: string, projectPath: string, projectGraph: MappedProjectGraph, - sourceFilePath: string, - sourceProject: ProjectGraphProjectNode -): boolean { + sourceFilePath: string +) { if (!isRelative(imp)) return false; const targetFile = normalizePath( path.resolve(path.join(projectPath, path.dirname(sourceFilePath)), imp) ).substring(projectPath.length + 1); - const targetProject = findTargetProject(projectGraph, targetFile); + return findTargetProject(projectGraph, targetFile); +} + +export function isRelativeImportIntoAnotherProject( + imp: string, + projectPath: string, + projectGraph: MappedProjectGraph, + sourceFilePath: string, + sourceProject: ProjectGraphProjectNode +): boolean { + const targetProject = getTargetProjectBasedOnRelativeImport( + imp, + projectPath, + projectGraph, + sourceFilePath + ); return sourceProject && targetProject && sourceProject !== targetProject; }