From 6ab43d73359adf38c70d9085e591acc88e6c257d Mon Sep 17 00:00:00 2001 From: George Kalpakas Date: Fri, 10 Apr 2020 15:13:20 +0300 Subject: [PATCH] fix(ngcc): correctly detect external files from nested `node_modules/` (#36559) Previously, when we needed to detect whether a file is external to a package, we only checked whether the relative path to the file from the package's root started with `..`. This would detect external imports when the packages were siblings (e.g. peer dependencies or hoisted to the top of `node_modules/` by the package manager), but would fail to detect imports from packages located in nested `node_modules/` as external. For example, importing `node_modules/foo/node_modules/bar` from a file in `node_modules/foo/` would be considered internal to the `foo` package. This could result in processing/analyzing more files than necessary. More importantly it could lead to errors due to trying to analyze non-Angular packages that were direct dependencies of Angular packages. This commit fixes it by also verifying that the relative path to a file does not start with `node_modules/`. Jira issue: [FW-2068](https://angular-team.atlassian.net/browse/FW-2068) Fixes #36526 PR Close #36559 --- .../compiler-cli/ngcc/src/analysis/util.ts | 3 +- .../test/analysis/decoration_analyzer_spec.ts | 30 +++++++--- .../ngcc/test/analysis/migration_host_spec.ts | 28 ++++++++- .../analysis/switch_marker_analyzer_spec.ts | 59 +++++++++++++------ .../ngcc/test/analysis/util_spec.ts | 24 ++++++-- 5 files changed, 109 insertions(+), 35 deletions(-) diff --git a/packages/compiler-cli/ngcc/src/analysis/util.ts b/packages/compiler-cli/ngcc/src/analysis/util.ts index ef4a3931da..fb51a2c7eb 100644 --- a/packages/compiler-cli/ngcc/src/analysis/util.ts +++ b/packages/compiler-cli/ngcc/src/analysis/util.ts @@ -11,7 +11,8 @@ import {absoluteFromSourceFile, AbsoluteFsPath, relative} from '../../../src/ngt import {DependencyTracker} from '../../../src/ngtsc/incremental/api'; export function isWithinPackage(packagePath: AbsoluteFsPath, sourceFile: ts.SourceFile): boolean { - return !relative(packagePath, absoluteFromSourceFile(sourceFile)).startsWith('..'); + const relativePath = relative(packagePath, absoluteFromSourceFile(sourceFile)); + return !relativePath.startsWith('..') && !relativePath.startsWith('node_modules/'); } class NoopDependencyTracker implements DependencyTracker { diff --git a/packages/compiler-cli/ngcc/test/analysis/decoration_analyzer_spec.ts b/packages/compiler-cli/ngcc/test/analysis/decoration_analyzer_spec.ts index 8ec0130a95..014eb81d9a 100644 --- a/packages/compiler-cli/ngcc/test/analysis/decoration_analyzer_spec.ts +++ b/packages/compiler-cli/ngcc/test/analysis/decoration_analyzer_spec.ts @@ -313,17 +313,31 @@ runInEachFileSystem(() => { contents: ` import {Component, NgModule} from '@angular/core'; import {ImportedComponent} from 'other/component'; + import {NestedDependencyComponent} from 'nested/component'; export class LocalComponent {} LocalComponent.decorators = [{type: Component}]; export class MyModule {} MyModule.decorators = [{type: NgModule, args: [{ - declarations: [ImportedComponent, LocalComponent], - exports: [ImportedComponent, LocalComponent], + declarations: [ImportedComponent, NestedDependencyComponent, LocalComponent], + exports: [ImportedComponent, NestedDependencyComponent, LocalComponent], },] }]; ` }, + // Do not define a `.d.ts` file to ensure that the `.js` file will be part of the TS + // program. + { + name: _('/node_modules/test-package/node_modules/nested/component.js'), + contents: ` + import {Component} from '@angular/core'; + export class NestedDependencyComponent {} + NestedDependencyComponent.decorators = [{type: Component}]; + `, + isRoot: false, + }, + // Do not define a `.d.ts` file to ensure that the `.js` file will be part of the TS + // program. { name: _('/node_modules/other/component.js'), contents: ` @@ -333,12 +347,6 @@ runInEachFileSystem(() => { `, isRoot: false, }, - { - name: _('/node_modules/other/component.d.ts'), - contents: ` - import {Component} from '@angular/core'; - export class ImportedComponent {}` - }, ]; const analyzer = setUpAnalyzer(EXTERNAL_COMPONENT_PROGRAM); @@ -349,6 +357,12 @@ runInEachFileSystem(() => { const file = program.getSourceFile(_('/node_modules/other/component.js'))!; expect(result.has(file)).toBe(false); }); + + it('should ignore classes from a file imported from a nested `node_modules/`', () => { + const file = program.getSourceFile( + _('/node_modules/test-package/node_modules/nested/component.js'))!; + expect(result.has(file)).toBe(false); + }); }); describe('diagnostic handling', () => { diff --git a/packages/compiler-cli/ngcc/test/analysis/migration_host_spec.ts b/packages/compiler-cli/ngcc/test/analysis/migration_host_spec.ts index 57c3da663f..3598496c11 100644 --- a/packages/compiler-cli/ngcc/test/analysis/migration_host_spec.ts +++ b/packages/compiler-cli/ngcc/test/analysis/migration_host_spec.ts @@ -92,8 +92,6 @@ runInEachFileSystem(() => { }); }); - - describe('getAllDecorators', () => { it('should include injected decorators', () => { const directiveHandler = new DetectDecoratorHandler('Directive', HandlerPrecedence.WEAK); @@ -143,7 +141,7 @@ runInEachFileSystem(() => { expect(host.isInScope(internalClass)).toBe(true); }); - it('should be false for nodes outside the entry-point', () => { + it('should be false for nodes outside the entry-point (in sibling package)', () => { loadTestFiles([ {name: _('/node_modules/external/index.js'), contents: `export class ExternalClass {}`}, { @@ -163,6 +161,30 @@ runInEachFileSystem(() => { expect(host.isInScope(externalClass)).toBe(false); }); + + it('should be false for nodes outside the entry-point (in nested `node_modules/`)', () => { + loadTestFiles([ + { + name: _('/node_modules/test/index.js'), + contents: ` + export {NestedDependencyClass} from 'nested'; + export class InternalClass {} + `, + }, + { + name: _('/node_modules/test/node_modules/nested/index.js'), + contents: `export class NestedDependencyClass {}`, + }, + ]); + const entryPoint = + makeTestEntryPointBundle('test', 'esm2015', false, [_('/node_modules/test/index.js')]); + const {host} = createMigrationHost({entryPoint, handlers: []}); + const nestedDepClass = getDeclaration( + entryPoint.src.program, _('/node_modules/test/node_modules/nested/index.js'), + 'NestedDependencyClass', isNamedClassDeclaration); + + expect(host.isInScope(nestedDepClass)).toBe(false); + }); }); }); }); diff --git a/packages/compiler-cli/ngcc/test/analysis/switch_marker_analyzer_spec.ts b/packages/compiler-cli/ngcc/test/analysis/switch_marker_analyzer_spec.ts index 53f91a7d46..403877d50c 100644 --- a/packages/compiler-cli/ngcc/test/analysis/switch_marker_analyzer_spec.ts +++ b/packages/compiler-cli/ngcc/test/analysis/switch_marker_analyzer_spec.ts @@ -24,45 +24,53 @@ runInEachFileSystem(() => { { name: _('/node_modules/test/entrypoint.js'), contents: ` - import {a} from './a'; - import {b} from './b'; - import {x} from '../other/x'; - ` + import {a} from './a'; + import {b} from './b'; + import {x} from '../other/x'; + import {e} from 'nested/e'; + `, }, { name: _('/node_modules/test/a.js'), contents: ` - import {c} from './c'; - export const a = 1; - ` + import {c} from './c'; + export const a = 1; + `, }, { name: _('/node_modules/test/b.js'), contents: ` - export const b = 42; - var factoryB = factory__PRE_R3__; - ` + export const b = 42; + var factoryB = factory__PRE_R3__; + `, }, { name: _('/node_modules/test/c.js'), contents: ` - export const c = 'So long, and thanks for all the fish!'; - var factoryC = factory__PRE_R3__; - var factoryD = factory__PRE_R3__; - ` + export const c = 'So long, and thanks for all the fish!'; + var factoryC = factory__PRE_R3__; + var factoryD = factory__PRE_R3__; + `, + }, + { + name: _('/node_modules/test/node_modules/nested/e.js'), + contents: ` + export const e = 1337; + var factoryE = factory__PRE_R3__; + `, }, { name: _('/node_modules/other/x.js'), contents: ` - export const x = 3.142; - var factoryX = factory__PRE_R3__; - ` + export const x = 3.142; + var factoryX = factory__PRE_R3__; + `, }, { name: _('/node_modules/other/x.d.ts'), contents: ` - export const x: number; - ` + export const x: number; + `, }, ]; }); @@ -111,6 +119,19 @@ runInEachFileSystem(() => { const x = getSourceFileOrError(program, _('/node_modules/other/x.js')); expect(analysis.has(x)).toBe(false); }); + + it('should ignore files that are inside the package\'s `node_modules/`', () => { + loadTestFiles(TEST_PROGRAM); + const bundle = makeTestEntryPointBundle( + 'test', 'esm2015', false, [_('/node_modules/test/entrypoint.js')]); + const program = bundle.src.program; + const host = new Esm2015ReflectionHost(new MockLogger(), false, bundle.src); + const analyzer = new SwitchMarkerAnalyzer(host, bundle.entryPoint.package); + const analysis = analyzer.analyzeProgram(program); + + const x = getSourceFileOrError(program, _('/node_modules/test/node_modules/nested/e.js')); + expect(analysis.has(x)).toBe(false); + }); }); }); }); diff --git a/packages/compiler-cli/ngcc/test/analysis/util_spec.ts b/packages/compiler-cli/ngcc/test/analysis/util_spec.ts index fd6ee82409..048deb0efe 100644 --- a/packages/compiler-cli/ngcc/test/analysis/util_spec.ts +++ b/packages/compiler-cli/ngcc/test/analysis/util_spec.ts @@ -12,20 +12,36 @@ import {isWithinPackage} from '../../src/analysis/util'; runInEachFileSystem(() => { describe('isWithinPackage', () => { + let _: typeof absoluteFrom; + + beforeEach(() => _ = absoluteFrom); + it('should return true if the source-file is contained in the package', () => { - const _ = absoluteFrom; + const packagePath = _('/node_modules/test'); const file = ts.createSourceFile(_('/node_modules/test/src/index.js'), '', ts.ScriptTarget.ES2015); - const packagePath = _('/node_modules/test'); expect(isWithinPackage(packagePath, file)).toBe(true); }); it('should return false if the source-file is not contained in the package', () => { - const _ = absoluteFrom; + const packagePath = _('/node_modules/test'); const file = ts.createSourceFile(_('/node_modules/other/src/index.js'), '', ts.ScriptTarget.ES2015); - const packagePath = _('/node_modules/test'); expect(isWithinPackage(packagePath, file)).toBe(false); }); + + it('should return false if the source-file is inside the package\'s `node_modules/`', () => { + const packagePath = _('/node_modules/test'); + + // An external file inside the package's `node_modules/`. + const file1 = ts.createSourceFile( + _('/node_modules/test/node_modules/other/src/index.js'), '', ts.ScriptTarget.ES2015); + expect(isWithinPackage(packagePath, file1)).toBe(false); + + // An internal file starting with `node_modules`. + const file2 = ts.createSourceFile( + _('/node_modules/test/node_modules_optimizer.js'), '', ts.ScriptTarget.ES2015); + expect(isWithinPackage(packagePath, file2)).toBe(true); + }); }); });