Skip to content

Commit e8ceae1

Browse files
devversionalxhub
devversion
authored and
alxhub
committedMay 8, 2019
fix(core): migrations not always migrating all files (#30269)
In an Angular CLI project scenario where projects only reference top-level source-files through the `tsconfig` `files` option, we currently do not migrate referenced source-files. This can be fixed checking all referenced source-files which aren't coming from an external library. This is similar to how `tslint` determines project source-files. PR Close #30269
·
8.0.38.0.0-rc.4
1 parent c3246e6 commit e8ceae1

File tree

4 files changed

+11
-7
lines changed

4 files changed

+11
-7
lines changed
 

‎packages/core/schematics/migrations/injectable-pipe/index.ts

+3-2
Original file line numberDiff line numberDiff line change
@@ -53,10 +53,11 @@ function runInjectablePipeMigration(tree: Tree, tsconfigPath: string, basePath:
5353
const program = ts.createProgram(parsed.fileNames, parsed.options, host);
5454
const typeChecker = program.getTypeChecker();
5555
const visitor = new InjectablePipeVisitor(typeChecker);
56-
const rootSourceFiles = program.getRootFileNames().map(f => program.getSourceFile(f) !);
56+
const sourceFiles = program.getSourceFiles().filter(
57+
f => !f.isDeclarationFile && !program.isSourceFileFromExternalLibrary(f));
5758
const printer = ts.createPrinter();
5859

59-
rootSourceFiles.forEach(sourceFile => visitor.visitNode(sourceFile));
60+
sourceFiles.forEach(sourceFile => visitor.visitNode(sourceFile));
6061

6162
visitor.missingInjectablePipes.forEach(data => {
6263
const {classDeclaration, importDeclarationMissingImport} = data;

‎packages/core/schematics/migrations/move-document/index.ts

+3-2
Original file line numberDiff line numberDiff line change
@@ -54,10 +54,11 @@ function runMoveDocumentMigration(tree: Tree, tsconfigPath: string, basePath: st
5454
const program = ts.createProgram(parsed.fileNames, parsed.options, host);
5555
const typeChecker = program.getTypeChecker();
5656
const visitor = new DocumentImportVisitor(typeChecker);
57-
const rootSourceFiles = program.getRootFileNames().map(f => program.getSourceFile(f) !);
57+
const sourceFiles = program.getSourceFiles().filter(
58+
f => !f.isDeclarationFile && !program.isSourceFileFromExternalLibrary(f));
5859

5960
// Analyze source files by finding imports.
60-
rootSourceFiles.forEach(sourceFile => visitor.visitNode(sourceFile));
61+
sourceFiles.forEach(sourceFile => visitor.visitNode(sourceFile));
6162

6263
const {importsMap} = visitor;
6364

‎packages/core/schematics/migrations/static-queries/index.ts

+2-1
Original file line numberDiff line numberDiff line change
@@ -117,7 +117,8 @@ function analyzeProject(tree: Tree, tsconfigPath: string, basePath: string):
117117

118118
const program = ts.createProgram(parsed.fileNames, parsed.options, host);
119119
const typeChecker = program.getTypeChecker();
120-
const sourceFiles = program.getRootFileNames().map(f => program.getSourceFile(f) !);
120+
const sourceFiles = program.getSourceFiles().filter(
121+
f => !f.isDeclarationFile && !program.isSourceFileFromExternalLibrary(f));
121122
const queryVisitor = new NgQueryResolveVisitor(typeChecker);
122123

123124
// Analyze all project source-files and collect all queries that

‎packages/core/schematics/migrations/template-var-assignment/index.ts

+3-2
Original file line numberDiff line numberDiff line change
@@ -60,10 +60,11 @@ function runTemplateVariableAssignmentCheck(
6060
const program = ts.createProgram(parsed.fileNames, parsed.options, host);
6161
const typeChecker = program.getTypeChecker();
6262
const templateVisitor = new NgComponentTemplateVisitor(typeChecker);
63-
const rootSourceFiles = program.getRootFileNames().map(f => program.getSourceFile(f) !);
63+
const sourceFiles = program.getSourceFiles().filter(
64+
f => !f.isDeclarationFile && !program.isSourceFileFromExternalLibrary(f));
6465

6566
// Analyze source files by detecting HTML templates.
66-
rootSourceFiles.forEach(sourceFile => templateVisitor.visitNode(sourceFile));
67+
sourceFiles.forEach(sourceFile => templateVisitor.visitNode(sourceFile));
6768

6869
const {resolvedTemplates} = templateVisitor;
6970
const collectedFailures: string[] = [];

2 commit comments

Comments
 (2)

IgorMinar commented on May 14, 2019

@IgorMinar
IgorMinar
on May 14, 2019
Contributor

@devversion if writing a test for this change is prohibitively expensive then please state so in the commit message, but in general whenever possible we should try to include tests for all changes.

devversion commented on May 14, 2019

@devversion
devversion
on May 14, 2019
MemberAuthor

@IgorMinar ops sorry for that. I usually try to do that for every change, but this one was slightly different. Will put a note in the commit message next time. Thanks for telling me 😄

For later reference: There is actually a test that depends on this logic to work, but it was part of a follow-up commit in the same PR (see a71d8a8). I agree that ideally there would be an explicit test that is part of this commit. We can have these tests but it will result in a bit duplication since we actually have this logic for every migration schematic and running it outside of a schematic test runner would not have much value.

Please sign in to comment.