From 850aee2448e01e4d43582fbf605eb8d7ef5b7370 Mon Sep 17 00:00:00 2001 From: Alex Rickabaugh Date: Wed, 13 Nov 2019 10:52:49 -0800 Subject: [PATCH] fix(ivy): emit fs-relative paths when rootDir(s) aren't in effect (#33828) Previously, the compiler assumed that all TS files logically within a project existed under one or more "root directories". If the TS compiler option `rootDir` or `rootDirs` was set, they would dictate the root directories in use, otherwise the current directory was used. Unfortunately this assumption was unfounded - it's common for projects without explicit `rootDirs` to import from files outside the current working directory. In such cases the `LogicalProjectStrategy` would attempt to generate imports into those files, and fail. This would lead to no `ReferenceEmitStrategy` being able to generate an import, and end in a compiler assertion failure. This commit introduces a new strategy to use when there are no `rootDirs` explicitly present, the `RelativePathStrategy`. It uses simpler, filesystem- relative paths to generate imports, even to files above the current working directory. Fixes #33659 Fixes #33562 PR Close #33828 --- .../compiler-cli/src/ngtsc/imports/README.md | 12 +- .../compiler-cli/src/ngtsc/imports/index.ts | 2 +- .../src/ngtsc/imports/src/emitter.ts | 29 ++++- packages/compiler-cli/src/ngtsc/program.ts | 32 +++-- .../compiler-cli/test/ngtsc/monorepo_spec.ts | 117 ++++++++++++++++++ 5 files changed, 181 insertions(+), 11 deletions(-) create mode 100644 packages/compiler-cli/test/ngtsc/monorepo_spec.ts diff --git a/packages/compiler-cli/src/ngtsc/imports/README.md b/packages/compiler-cli/src/ngtsc/imports/README.md index 9f1e5bae7a..08f31632b6 100644 --- a/packages/compiler-cli/src/ngtsc/imports/README.md +++ b/packages/compiler-cli/src/ngtsc/imports/README.md @@ -68,7 +68,17 @@ This `ReferenceEmitStrategy` queries the `Reference` for a `ts.Identifier` that' ### `LogicalProjectStrategy` -This `ReferenceEmitStrategy` is used to import referenced classes that are declared in the current project, and not in any third-party or external libraries. It constructs an import path that's valid within the logical filesystem of the project, even if the project has multiple `rootDirs`. +This `ReferenceEmitStrategy` is used to import referenced classes that are declared in the current project, and not in any third-party or external libraries, whenever `rootDir` or `rootDirs` is set in the TS compiler options. + +When `rootDir`(s) are present, multiple physical directories can be mapped into the same logical namespace. So consider two files `/app/app.cmp.ts` and `/lib/lib.cmp.ts`. Ordinarily, a relative import (such as the kind generated by `RelativePathStrategy`) from the former to the latter would be `../lib/lib.cmp`. However, if both `/app` and `/lib` are project `rootDirs`, then the files within are logically in the same "directory", and the correct import is `./lib.cmp`. + +The `LogicalProjectStrategy` constructs `LogicalProjectPath`s between files and generates module specifiers that are relative imports within that namespace, honoring the project's `rootDirs` settings. + +The `LogicalProjectStrategy` will decline to generate an import into any file which falls outside the project's `rootDirs`, as such a relative specifier is not representable in the merged namespace. + +### `RelativePathStrategy` + +This `ReferenceEmitStrategy` is used to generate relative imports between two files in the project, assuming the layout of files on the disk maps directly to the module specifier namespace. This is the case if the project does not have `rootDir`/`rootDirs` configured in its TS compiler options. ### `AbsoluteModuleStrategy` diff --git a/packages/compiler-cli/src/ngtsc/imports/index.ts b/packages/compiler-cli/src/ngtsc/imports/index.ts index 847cb09e88..446aa54cc7 100644 --- a/packages/compiler-cli/src/ngtsc/imports/index.ts +++ b/packages/compiler-cli/src/ngtsc/imports/index.ts @@ -9,7 +9,7 @@ export {AliasStrategy, AliasingHost, FileToModuleAliasingHost, PrivateExportAliasingHost} from './src/alias'; export {ImportRewriter, NoopImportRewriter, R3SymbolsImportRewriter, validateAndRewriteCoreSymbol} from './src/core'; export {DefaultImportRecorder, DefaultImportTracker, NOOP_DEFAULT_IMPORT_RECORDER} from './src/default'; -export {AbsoluteModuleStrategy, FileToModuleHost, FileToModuleStrategy, LocalIdentifierStrategy, LogicalProjectStrategy, ReferenceEmitStrategy, ReferenceEmitter} from './src/emitter'; +export {AbsoluteModuleStrategy, FileToModuleHost, FileToModuleStrategy, LocalIdentifierStrategy, LogicalProjectStrategy, ReferenceEmitStrategy, ReferenceEmitter, RelativePathStrategy} from './src/emitter'; export {Reexport} from './src/reexport'; export {ImportMode, OwningModule, Reference} from './src/references'; export {ModuleResolver} from './src/resolver'; diff --git a/packages/compiler-cli/src/ngtsc/imports/src/emitter.ts b/packages/compiler-cli/src/ngtsc/imports/src/emitter.ts index 3ee0a0848b..4fb3c5880b 100644 --- a/packages/compiler-cli/src/ngtsc/imports/src/emitter.ts +++ b/packages/compiler-cli/src/ngtsc/imports/src/emitter.ts @@ -7,12 +7,17 @@ */ import {Expression, ExternalExpr, ExternalReference, WrappedNodeExpr} from '@angular/compiler'; import * as ts from 'typescript'; -import {LogicalFileSystem, LogicalProjectPath, absoluteFrom} from '../../file_system'; + +import {LogicalFileSystem, LogicalProjectPath, PathSegment, absoluteFrom, absoluteFromSourceFile, basename, dirname, relative, resolve} from '../../file_system'; +import {stripExtension} from '../../file_system/src/util'; import {ReflectionHost} from '../../reflection'; import {getSourceFile, getSourceFileOrNull, isDeclaration, nodeNameForError, resolveModuleName} from '../../util/src/typescript'; + import {findExportedNameOfNode} from './find_export'; import {ImportMode, Reference} from './references'; + + /** * A host which supports an operation to convert a file name into a module name. * @@ -233,6 +238,28 @@ export class LogicalProjectStrategy implements ReferenceEmitStrategy { } } +/** + * A `ReferenceEmitStrategy` which constructs relatives paths between `ts.SourceFile`s. + * + * This strategy can be used if there is no `rootDir`/`rootDirs` structure for the project which + * necessitates the stronger logic of `LogicalProjectStrategy`. + */ +export class RelativePathStrategy implements ReferenceEmitStrategy { + constructor(private reflector: ReflectionHost) {} + + emit(ref: Reference, context: ts.SourceFile): Expression|null { + const destSf = getSourceFile(ref.node); + let moduleName = stripExtension( + relative(dirname(absoluteFromSourceFile(context)), absoluteFromSourceFile(destSf))); + if (!moduleName.startsWith('../')) { + moduleName = ('./' + moduleName) as PathSegment; + } + + const name = findExportedNameOfNode(ref.node, destSf, this.reflector); + return new ExternalExpr({moduleName, name}); + } +} + /** * A `ReferenceEmitStrategy` which uses a `FileToModuleHost` to generate absolute import references. */ diff --git a/packages/compiler-cli/src/ngtsc/program.ts b/packages/compiler-cli/src/ngtsc/program.ts index 96e571e81e..39d4d355d4 100644 --- a/packages/compiler-cli/src/ngtsc/program.ts +++ b/packages/compiler-cli/src/ngtsc/program.ts @@ -18,7 +18,7 @@ import {CycleAnalyzer, ImportGraph} from './cycles'; import {ErrorCode, ngErrorCode} from './diagnostics'; import {FlatIndexGenerator, ReferenceGraph, checkForPrivateExports, findFlatIndexEntryPoint} from './entry_point'; import {AbsoluteFsPath, LogicalFileSystem, absoluteFrom} from './file_system'; -import {AbsoluteModuleStrategy, AliasStrategy, AliasingHost, DefaultImportTracker, FileToModuleAliasingHost, FileToModuleHost, FileToModuleStrategy, ImportRewriter, LocalIdentifierStrategy, LogicalProjectStrategy, ModuleResolver, NoopImportRewriter, PrivateExportAliasingHost, R3SymbolsImportRewriter, Reference, ReferenceEmitter} from './imports'; +import {AbsoluteModuleStrategy, AliasStrategy, AliasingHost, DefaultImportTracker, FileToModuleAliasingHost, FileToModuleHost, FileToModuleStrategy, ImportRewriter, LocalIdentifierStrategy, LogicalProjectStrategy, ModuleResolver, NoopImportRewriter, PrivateExportAliasingHost, R3SymbolsImportRewriter, Reference, ReferenceEmitStrategy, ReferenceEmitter, RelativePathStrategy} from './imports'; import {IncrementalState} from './incremental'; import {IndexedComponent, IndexingContext} from './indexer'; import {generateAnalysis} from './indexer/src/transform'; @@ -520,6 +520,24 @@ export class NgtscProgram implements api.Program { // Construct the ReferenceEmitter. if (this.fileToModuleHost === null || !this.options._useHostForImportGeneration) { + let localImportStrategy: ReferenceEmitStrategy; + + // The strategy used for local, in-project imports depends on whether TS has been configured + // with rootDirs. If so, then multiple directories may be mapped in the same "module + // namespace" and the logic of `LogicalProjectStrategy` is required to generate correct + // imports which may cross these multiple directories. Otherwise, plain relative imports are + // sufficient. + if (this.options.rootDir !== undefined || + (this.options.rootDirs !== undefined && this.options.rootDirs.length > 0)) { + // rootDirs logic is in effect - use the `LogicalProjectStrategy` for in-project relative + // imports. + localImportStrategy = + new LogicalProjectStrategy(this.reflector, new LogicalFileSystem(this.rootDirs)); + } else { + // Plain relative imports are all that's needed. + localImportStrategy = new RelativePathStrategy(this.reflector); + } + // The CompilerHost doesn't have fileNameToModuleName, so build an NPM-centric reference // resolution strategy. this.refEmitter = new ReferenceEmitter([ @@ -528,10 +546,10 @@ export class NgtscProgram implements api.Program { // Next, attempt to use an absolute import. new AbsoluteModuleStrategy( this.tsProgram, checker, this.options, this.host, this.reflector), - // Finally, check if the reference is being written into a file within the project's logical - // file system, and use a relative import if so. If this fails, ReferenceEmitter will throw + // Finally, check if the reference is being written into a file within the project's .ts + // sources, and use a relative import if so. If this fails, ReferenceEmitter will throw // an error. - new LogicalProjectStrategy(this.reflector, new LogicalFileSystem(this.rootDirs)), + localImportStrategy, ]); // If an entrypoint is present, then all user imports should be directed through the @@ -569,10 +587,8 @@ export class NgtscProgram implements api.Program { this.metaReader = new CompoundMetadataReader([localMetaReader, dtsReader]); - // If a flat module entrypoint was specified, then track references via a `ReferenceGraph` - // in - // order to produce proper diagnostics for incorrectly exported directives/pipes/etc. If - // there + // If a flat module entrypoint was specified, then track references via a `ReferenceGraph` in + // order to produce proper diagnostics for incorrectly exported directives/pipes/etc. If there // is no flat module entrypoint then don't pay the cost of tracking references. let referencesRegistry: ReferencesRegistry; if (this.entryPoint !== null) { diff --git a/packages/compiler-cli/test/ngtsc/monorepo_spec.ts b/packages/compiler-cli/test/ngtsc/monorepo_spec.ts new file mode 100644 index 0000000000..ac90576641 --- /dev/null +++ b/packages/compiler-cli/test/ngtsc/monorepo_spec.ts @@ -0,0 +1,117 @@ +/** + * @license + * Copyright Google Inc. All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + +import {absoluteFrom} from '../../src/ngtsc/file_system'; +import {runInEachFileSystem} from '../../src/ngtsc/file_system/testing'; +import {loadStandardTestFiles} from '../helpers/src/mock_file_loading'; + +import {NgtscTestEnvironment} from './env'; + +const testFiles = loadStandardTestFiles(); + +runInEachFileSystem(() => { + describe('monorepos', () => { + let env !: NgtscTestEnvironment; + + beforeEach(() => { + env = NgtscTestEnvironment.setup(testFiles, absoluteFrom('/app')); + env.tsconfig(); + + // env.tsconfig() will write to /app/tsconfig.json, but list it as extending + // ./tsconfig-base.json. So that file should exist, and redirect to ../tsconfig-base.json. + env.write('/app/tsconfig-base.json', JSON.stringify({'extends': '../tsconfig-base.json'})); + }); + + it('should compile a project with a reference above the current dir', () => { + env.write('/app/index.ts', ` + import {Component, NgModule} from '@angular/core'; + import {LibModule} from '../lib/module'; + + @Component({ + selector: 'app-cmp', + template: '', + }) + export class AppCmp {} + + @NgModule({ + declarations: [AppCmp], + imports: [LibModule], + }) + export class AppModule {} + `); + env.write('/lib/module.ts', ` + import {NgModule} from '@angular/core'; + import {LibCmp} from './cmp'; + + @NgModule({ + declarations: [LibCmp], + exports: [LibCmp], + }) + export class LibModule {} + `); + env.write('/lib/cmp.ts', ` + import {Component} from '@angular/core'; + + @Component({ + selector: 'lib-cmp', + template: '...', + }) + export class LibCmp {} + `); + + env.driveMain(); + + const jsContents = env.getContents('app/index.js'); + expect(jsContents).toContain(`import * as i1 from "../lib/cmp";`); + }); + + it('should compile a project with a reference into the same dir', () => { + env.write('/app/index.ts', ` + import {Component, NgModule} from '@angular/core'; + import {TargetModule} from './target'; + + @Component({ + selector: 'app-cmp', + template: '', + }) + export class AppCmp {} + + @NgModule({ + declarations: [AppCmp], + imports: [TargetModule], + }) + export class AppModule {} + `); + + env.write('/app/target.ts', ` + import {Component, NgModule} from '@angular/core'; + + @Component({ + selector: 'target-cmp', + template: '...', + }) + export class TargetCmp {} + + @NgModule({ + declarations: [TargetCmp], + exports: [TargetCmp], + }) + export class TargetModule {} + `); + + env.driveMain(); + + // Look for index.js, not app/index.js, because of TypeScript's behavior of stripping off the + // common prefix of all input files. + const jsContents = env.getContents('index.js'); + + // The real goal of this test was to check that the relative import has the leading './'. + expect(jsContents).toContain(`import * as i1 from "./target";`); + }); + }); +});