cargdev/angular
1
0
Fork 0
Code Issues 18 Pull Requests Actions Packages Projects Releases Wiki Activity

fix(ivy): ngcc - resolve main property paths correctly (#31509)

Browse Source 
There are two places in the ngcc processing where it needs to load the
content of a file given by a general path:

* when determining the format of an entry-point.
 To do this ngcc uses the value of the relevant property in package.json.
 But in the case of `main` it must parse the contents of the entry-point
 file to decide whether the format is UMD or CommonJS.

* when parsing the source files for dependencies to determine the order in
which compilation must occur. The relative imports in each file are parsed
and followed recursively, looking for external imports.

Previously, we naively assumed that the path would match the file name exactly.
But actually we must consider the standard module resolution conventions.
E.g. the extension (.js) may be missing, or the path may refer to a directory
containing an index.js file.

This commit fixes both places.

This commit now requires the `DependencyHost` instances to check
the existence of more files than before (at worst all the different possible
post-fixes). This should not create a significant performance reduction for
ngcc. Since the results of the checks will be cached, and similar work is
done inside the TS compiler, so what we lose in doing it here, is saved later
in the processing. The main performance loss would be where there are lots
of files that need to be parsed for dependencies that do not end up being
processed by TS. But compared to the main ngcc processing this dependency
parsing is a small proportion of the work done and so should not impact
much on the overall performance of ngcc.

// FW-1444

PR Close #31509
This commit is contained in:
Pete Bacon Darwin
2019-07-11 12:34:45 +01:00
committed by Matias Niemelä
parent 63e458dd3a
commit fac20bd8d1
11 changed files with 286 additions and 173 deletions
Download Patch File Download Diff File Expand all files Collapse all files

55
packages/compiler-cli/ngcc/test/dependencies/esm_dependency_host_spec.ts
View File

@ -10,7 +10,7 @@ import * as ts from 'typescript';
import {absoluteFrom, getFileSystem, relativeFrom} from '../../../src/ngtsc/file_system';
import {runInEachFileSystem} from '../../../src/ngtsc/file_system/testing';
import {loadTestFiles} from '../../../test/helpers';
import {EsmDependencyHost} from '../../src/dependencies/esm_dependency_host';
import {EsmDependencyHost, hasImportOrReexportStatements, isStringImportOrReexport} from '../../src/dependencies/esm_dependency_host';
import {ModuleResolver} from '../../src/dependencies/module_resolver';
runInEachFileSystem(() => {
@ -116,6 +116,16 @@ runInEachFileSystem(() => {
expect(missing.size).toBe(0);
expect(deepImports.size).toBe(0);
});
it('should handle entry-point paths with no extension', () => {
const {dependencies, missing, deepImports} =
host.findDependencies(_('/external/imports/index'));
expect(dependencies.size).toBe(2);
expect(missing.size).toBe(0);
expect(deepImports.size).toBe(0);
expect(dependencies.has(_('/node_modules/lib-1'))).toBe(true);
expect(dependencies.has(_('/node_modules/lib-1/sub-1'))).toBe(true);
});
});
function setupMockFileSystem(): void {
@ -210,24 +220,20 @@ runInEachFileSystem(() => {
describe('isStringImportOrReexport', () => {
it('should return true if the statement is an import', () => {
expect(host.isStringImportOrReexport(createStatement('import {X} from "some/x";')))
.toBe(true);
expect(host.isStringImportOrReexport(createStatement('import * as X from "some/x";')))
expect(isStringImportOrReexport(createStatement('import {X} from "some/x";'))).toBe(true);
expect(isStringImportOrReexport(createStatement('import * as X from "some/x";')))
.toBe(true);
});
it('should return true if the statement is a re-export', () => {
expect(host.isStringImportOrReexport(createStatement('export {X} from "some/x";')))
.toBe(true);
expect(host.isStringImportOrReexport(createStatement('export * from "some/x";')))
.toBe(true);
expect(isStringImportOrReexport(createStatement('export {X} from "some/x";'))).toBe(true);
expect(isStringImportOrReexport(createStatement('export * from "some/x";'))).toBe(true);
});
it('should return false if the statement is not an import or a re-export', () => {
expect(host.isStringImportOrReexport(createStatement('class X {}'))).toBe(false);
expect(host.isStringImportOrReexport(createStatement('export function foo() {}')))
.toBe(false);
expect(host.isStringImportOrReexport(createStatement('export const X = 10;'))).toBe(false);
expect(isStringImportOrReexport(createStatement('class X {}'))).toBe(false);
expect(isStringImportOrReexport(createStatement('export function foo() {}'))).toBe(false);
expect(isStringImportOrReexport(createStatement('export const X = 10;'))).toBe(false);
});
function createStatement(source: string) {
@ -239,28 +245,25 @@ runInEachFileSystem(() => {
describe('hasImportOrReexportStatements', () => {
it('should return true if there is an import statement', () => {
expect(host.hasImportOrReexportStatements('import {X} from "some/x";')).toBe(true);
expect(host.hasImportOrReexportStatements('import * as X from "some/x";')).toBe(true);
expect(host.hasImportOrReexportStatements(
'blah blah\n\n import {X} from "some/x";\nblah blah'))
expect(hasImportOrReexportStatements('import {X} from "some/x";')).toBe(true);
expect(hasImportOrReexportStatements('import * as X from "some/x";')).toBe(true);
expect(hasImportOrReexportStatements('blah blah\n\n import {X} from "some/x";\nblah blah'))
.toBe(true);
expect(host.hasImportOrReexportStatements('\t\timport {X} from "some/x";')).toBe(true);
expect(hasImportOrReexportStatements('\t\timport {X} from "some/x";')).toBe(true);
});
it('should return true if there is a re-export statement', () => {
expect(host.hasImportOrReexportStatements('export {X} from "some/x";')).toBe(true);
expect(host.hasImportOrReexportStatements(
'blah blah\n\n export {X} from "some/x";\nblah blah'))
expect(hasImportOrReexportStatements('export {X} from "some/x";')).toBe(true);
expect(hasImportOrReexportStatements('blah blah\n\n export {X} from "some/x";\nblah blah'))
.toBe(true);
expect(host.hasImportOrReexportStatements('\t\texport {X} from "some/x";')).toBe(true);
expect(host.hasImportOrReexportStatements(
expect(hasImportOrReexportStatements('\t\texport {X} from "some/x";')).toBe(true);
expect(hasImportOrReexportStatements(
'blah blah\n\n export * from "@angular/core;\nblah blah'))
.toBe(true);
});
it('should return false if there is no import nor re-export statement', () => {
expect(host.hasImportOrReexportStatements('blah blah')).toBe(false);
expect(host.hasImportOrReexportStatements('export function moo() {}')).toBe(false);
expect(
host.hasImportOrReexportStatements('Some text that happens to include the word import'))
expect(hasImportOrReexportStatements('blah blah')).toBe(false);
expect(hasImportOrReexportStatements('export function moo() {}')).toBe(false);
expect(hasImportOrReexportStatements('Some text that happens to include the word import'))
.toBe(false);
});
});