honeymoose
/
angular-cn
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

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
This commit is contained in:
George Kalpakas 2020-04-10 15:13:20 +03:00 committed by atscott
parent fee316161d
commit 6ab43d7335
5 changed files with 109 additions and 35 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
packages/compiler-cli/ngcc/src/analysis/util.ts
View File

@ -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 {

30
packages/compiler-cli/ngcc/test/analysis/decoration_analyzer_spec.ts
View File

@ -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', () => {

28
packages/compiler-cli/ngcc/test/analysis/migration_host_spec.ts
View File

@ -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);
});
});
});
});

59
packages/compiler-cli/ngcc/test/analysis/switch_marker_analyzer_spec.ts
View File

@ -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);
});
});
});
});

24
packages/compiler-cli/ngcc/test/analysis/util_spec.ts
View File

@ -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);
});
});
});