diff --git a/packages/compiler-cli/ngcc/src/packages/entry_point_bundle.ts b/packages/compiler-cli/ngcc/src/packages/entry_point_bundle.ts index 41ba3d33ea..68eea9495d 100644 --- a/packages/compiler-cli/ngcc/src/packages/entry_point_bundle.ts +++ b/packages/compiler-cli/ngcc/src/packages/entry_point_bundle.ts @@ -50,7 +50,7 @@ export function makeEntryPointBundle( const rootDir = entryPoint.packagePath; const options: ts .CompilerOptions = {allowJs: true, maxNodeModuleJsDepth: Infinity, rootDir, ...pathMappings}; - const srcHost = new NgccSourcesCompilerHost(fs, options, entryPoint.path); + const srcHost = new NgccSourcesCompilerHost(fs, options, entryPoint.packagePath); const dtsHost = new NgtscCompilerHost(fs, options); // Create the bundle programs, as necessary. @@ -63,7 +63,7 @@ export function makeEntryPointBundle( []; const dts = transformDts ? makeBundleProgram( fs, isCore, entryPoint.packagePath, typingsPath, 'r3_symbols.d.ts', - options, dtsHost, additionalDtsFiles) : + {...options, allowJs: false}, dtsHost, additionalDtsFiles) : null; const isFlatCore = isCore && src.r3SymbolsFile === null; diff --git a/packages/compiler-cli/ngcc/src/packages/ngcc_compiler_host.ts b/packages/compiler-cli/ngcc/src/packages/ngcc_compiler_host.ts index aab8b01b57..4c5d62f767 100644 --- a/packages/compiler-cli/ngcc/src/packages/ngcc_compiler_host.ts +++ b/packages/compiler-cli/ngcc/src/packages/ngcc_compiler_host.ts @@ -7,7 +7,8 @@ */ import * as ts from 'typescript'; -import {FileSystem, NgtscCompilerHost} from '../../../src/ngtsc/file_system'; +import {AbsoluteFsPath, FileSystem, NgtscCompilerHost} from '../../../src/ngtsc/file_system'; +import {isWithinPackage} from '../analysis/util'; import {isRelativePath} from '../utils'; /** @@ -20,7 +21,7 @@ export class NgccSourcesCompilerHost extends NgtscCompilerHost { private cache = ts.createModuleResolutionCache( this.getCurrentDirectory(), file => this.getCanonicalFileName(file)); - constructor(fs: FileSystem, options: ts.CompilerOptions, protected entryPointPath: string) { + constructor(fs: FileSystem, options: ts.CompilerOptions, protected packagePath: AbsoluteFsPath) { super(fs, options); } @@ -36,13 +37,24 @@ export class NgccSourcesCompilerHost extends NgtscCompilerHost { // file was in the same directory. This is undesirable, as we need to have the actual // JavaScript being present in the program. This logic recognizes this scenario and rewrites // the resolved .d.ts declaration file to its .js counterpart, if it exists. - if (resolvedModule !== undefined && resolvedModule.extension === ts.Extension.Dts && - containingFile.endsWith('.js') && isRelativePath(moduleName)) { + if (resolvedModule?.extension === ts.Extension.Dts && containingFile.endsWith('.js') && + isRelativePath(moduleName)) { const jsFile = resolvedModule.resolvedFileName.replace(/\.d\.ts$/, '.js'); if (this.fileExists(jsFile)) { return {...resolvedModule, resolvedFileName: jsFile, extension: ts.Extension.Js}; } } + + // Prevent loading JavaScript source files outside of the package root, which would happen for + // packages that don't have .d.ts files. As ngcc should only operate on the .js files + // contained within the package, any files outside the package are simply discarded. This does + // result in a partial program with error diagnostics, however ngcc won't gather diagnostics + // for the program it creates so these diagnostics won't be reported. + if (resolvedModule?.extension === ts.Extension.Js && + !isWithinPackage(this.packagePath, this.fs.resolve(resolvedModule.resolvedFileName))) { + return undefined; + } + return resolvedModule; }); } diff --git a/packages/compiler-cli/ngcc/test/helpers/utils.ts b/packages/compiler-cli/ngcc/test/helpers/utils.ts index d0a7940671..75a2380554 100644 --- a/packages/compiler-cli/ngcc/test/helpers/utils.ts +++ b/packages/compiler-cli/ngcc/test/helpers/utils.ts @@ -68,7 +68,7 @@ export function makeTestBundleProgram( const rootDir = fs.dirname(entryPointPath); const options: ts.CompilerOptions = {allowJs: true, maxNodeModuleJsDepth: Infinity, checkJs: false, rootDir, rootDirs: [rootDir]}; - const host = new NgccSourcesCompilerHost(fs, options, entryPointPath); + const host = new NgccSourcesCompilerHost(fs, options, rootDir); return makeBundleProgram( fs, isCore, rootDir, path, 'r3_symbols.js', options, host, additionalFiles); } diff --git a/packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts b/packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts index a6531a5172..edbc732621 100644 --- a/packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts +++ b/packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts @@ -401,6 +401,121 @@ runInEachFileSystem(() => { expect(es5Contents).toContain('ɵngcc0.ɵɵtext(0, "a - b - 3 - 4")'); }); + it('should not crash when scanning for ModuleWithProviders needs to evaluate code from an external package', + () => { + // Regression test for https://github.com/angular/angular/issues/37508 + // During `ModuleWithProviders` analysis, return statements in methods are evaluated using + // the partial evaluator to identify whether they correspond with a `ModuleWithProviders` + // function. If an arbitrary method has a return statement that calls into an external + // module which doesn't have declaration files, ngcc would attempt to reflect on said + // module using the reflection host of the entry-point. This would crash in the case where + // e.g. the entry-point is UMD and the external module would be CommonJS, as the UMD + // reflection host would throw because it is unable to deal with CommonJS. + + // Setup a non-TS package with CommonJS module format + loadTestFiles([ + { + name: _(`/node_modules/identity/package.json`), + contents: `{"name": "identity", "main": "./index.js"}`, + }, + { + name: _(`/node_modules/identity/index.js`), + contents: ` + function identity(x) { return x; }; + exports.identity = identity; + module.exports = identity; + `, + }, + ]); + + // Setup an Angular entry-point with UMD module format that references an export of the + // CommonJS package. + loadTestFiles([ + { + name: _('/node_modules/test-package/package.json'), + contents: '{"name": "test-package", "main": "./index.js", "typings": "./index.d.ts"}' + }, + { + name: _('/node_modules/test-package/index.js'), + contents: ` + (function (global, factory) { + typeof exports === 'object' && typeof module !== 'undefined' ? factory(exports, require('identity')) : + typeof define === 'function' && define.amd ? define('test', ['exports', 'identity'], factory) : + (factory(global.test, global.identity)); + }(this, (function (exports, identity) { 'use strict'; + function Foo(x) { + // The below statement is analyzed for 'ModuleWithProviders', so is evaluated + // by ngcc. The reference into the non-TS CommonJS package used to crash ngcc. + return identity.identity(x); + } + exports.Foo = Foo; + }))); + ` + }, + { + name: _('/node_modules/test-package/index.d.ts'), + contents: 'export declare class Foo { static doSomething(x: any): any; }' + }, + {name: _('/node_modules/test-package/index.metadata.json'), contents: 'DUMMY DATA'}, + ]); + + expect(() => mainNgcc({ + basePath: '/node_modules', + targetEntryPointPath: 'test-package', + propertiesToConsider: ['main'], + })) + .not.toThrow(); + }); + + it('should not be able to evaluate code in external packages when no .d.ts files are present', + () => { + loadTestFiles([ + { + name: _(`/node_modules/external/package.json`), + contents: `{"name": "external", "main": "./index.js"}`, + }, + { + name: _(`/node_modules/external/index.js`), + contents: ` + export const selector = 'my-selector'; + `, + }, + ]); + + compileIntoApf('test-package', { + '/index.ts': ` + import {NgModule, Component} from '@angular/core'; + import {selector} from 'external'; + + @Component({ + selector, + template: '' + }) + export class FooComponent { + } + + @NgModule({ + declarations: [FooComponent], + }) + export class FooModule {} + `, + }); + + try { + mainNgcc({ + basePath: '/node_modules', + targetEntryPointPath: 'test-package', + propertiesToConsider: ['esm2015', 'esm5'], + }); + fail('should have thrown'); + } catch (e) { + expect(e.message).toContain( + 'Failed to compile entry-point test-package (esm2015 as esm2015) due to compilation errors:'); + expect(e.message).toContain('NG1010'); + expect(e.message).toContain('selector must be a string'); + } + }); + it('should add ɵfac but not duplicate ɵprov properties on injectables', () => { compileIntoFlatEs2015Package('test-package', { '/index.ts': ` diff --git a/packages/compiler-cli/ngcc/test/packages/entry_point_bundle_spec.ts b/packages/compiler-cli/ngcc/test/packages/entry_point_bundle_spec.ts index 546370ff53..8eb133ed0a 100644 --- a/packages/compiler-cli/ngcc/test/packages/entry_point_bundle_spec.ts +++ b/packages/compiler-cli/ngcc/test/packages/entry_point_bundle_spec.ts @@ -13,8 +13,12 @@ import {makeEntryPointBundle} from '../../src/packages/entry_point_bundle'; runInEachFileSystem(() => { describe('entry point bundle', () => { + let _: typeof absoluteFrom; + beforeEach(() => { + _ = absoluteFrom; + }); + function setupMockFileSystem(): void { - const _ = absoluteFrom; loadTestFiles([ { name: _('/node_modules/test/package.json'), @@ -210,6 +214,103 @@ runInEachFileSystem(() => { ].map(p => absoluteFrom(p).toString()))); }); + it('does not include .js files outside of the package when no .d.ts file is available', () => { + // Declare main "test" package with "entry" entry-point that imports all sorts of + // internal and external modules. + loadTestFiles([ + { + name: _('/node_modules/test/entry/package.json'), + contents: `{"name": "test", "main": "./index.js", "typings": "./index.d.ts"}`, + }, + { + name: _('/node_modules/test/entry/index.d.ts'), + contents: ` + import 'external-js'; + import 'external-ts'; + import 'nested-js'; + import './local'; + import '../package'; + `, + }, + { + name: _('/node_modules/test/entry/index.js'), + contents: ` + import 'external-js'; + import 'external-ts'; + import 'nested-js'; + import './local'; + import '../package'; + `, + }, + {name: _('/node_modules/test/entry/local.d.ts'), contents: `export {};`}, + {name: _('/node_modules/test/entry/local.js'), contents: `export {};`}, + {name: _('/node_modules/test/package.d.ts'), contents: `export {};`}, + {name: _('/node_modules/test/package.js'), contents: `export {};`}, + ]); + + // Declare "external-js" package outside of the "test" package without .d.ts files, should + // not be included in the program. + loadTestFiles([ + { + name: _('/node_modules/external-js/package.json'), + contents: `{"name": "external-js", "main": "./index.js"}`, + }, + {name: _('/node_modules/external-js/index.js'), contents: 'export {};'}, + ]); + + // Same as "external-js" but located in a nested node_modules directory, which should also + // not be included in the program. + loadTestFiles([ + { + name: _('/node_modules/test/node_modules/nested-js/package.json'), + contents: `{"name": "nested-js", "main": "./index.js"}`, + }, + {name: _('/node_modules/test/node_modules/nested-js/index.js'), contents: 'export {}'}, + ]); + + // Declare "external-ts" which does have .d.ts files, so the .d.ts should be + // loaded into the program. + loadTestFiles([ + { + name: _('/node_modules/external-ts/package.json'), + contents: `{"name": "external-ts", "main": "./index.js", "typings": "./index.d.ts"}`, + }, + {name: _('/node_modules/external-ts/index.d.ts'), contents: 'export {};'}, + {name: _('/node_modules/external-ts/index.js'), contents: 'export {};'}, + ]); + + const fs = getFileSystem(); + const entryPoint: EntryPoint = { + name: 'test/entry', + path: absoluteFrom('/node_modules/test/entry'), + packageName: 'test', + packagePath: absoluteFrom('/node_modules/test'), + packageJson: {name: 'test/entry'}, + typings: absoluteFrom('/node_modules/test/entry/index.d.ts'), + compiledByAngular: true, + ignoreMissingDependencies: false, + generateDeepReexports: false, + }; + const esm5bundle = makeEntryPointBundle( + fs, entryPoint, './index.js', false, 'esm5', /* transformDts */ true, + /* pathMappings */ undefined, /* mirrorDtsFromSrc */ true); + + expect(esm5bundle.src.program.getSourceFiles().map(sf => _(sf.fileName))) + .toEqual(jasmine.arrayWithExactContents([ + _('/node_modules/test/entry/index.js'), + _('/node_modules/test/entry/local.js'), + _('/node_modules/test/package.js'), + _('/node_modules/external-ts/index.d.ts'), + ])); + expect(esm5bundle.dts!.program.getSourceFiles().map(sf => _(sf.fileName))) + .toEqual(jasmine.arrayWithExactContents([ + _('/node_modules/test/entry/index.d.ts'), + _('/node_modules/test/entry/local.d.ts'), + _('/node_modules/test/package.d.ts'), + _('/node_modules/external-ts/index.d.ts'), + ])); + }); + describe( 'including equivalently named, internally imported, src files in the typings program', () => {