Skip to content

Commit

Permalink
fix(core): migrations not always migrating all files (#30269)
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@devversion @alxhub
devversion authored and alxhub committed May 8, 2019
1 parent c3246e6 commit e8ceae1
Show file tree
Hide file tree
Showing 4 changed files with 11 additions and 7 deletions.
5 changes: 3 additions & 2 deletions packages/core/schematics/migrations/injectable-pipe/index.ts
View file
Expand Up @@ -53,10 +53,11 @@ function runInjectablePipeMigration(tree: Tree, tsconfigPath: string, basePath:
const program = ts.createProgram(parsed.fileNames, parsed.options, host);
const typeChecker = program.getTypeChecker();
const visitor = new InjectablePipeVisitor(typeChecker);
const rootSourceFiles = program.getRootFileNames().map(f => program.getSourceFile(f) !);
const sourceFiles = program.getSourceFiles().filter(
f => !f.isDeclarationFile && !program.isSourceFileFromExternalLibrary(f));
const printer = ts.createPrinter();

rootSourceFiles.forEach(sourceFile => visitor.visitNode(sourceFile));
sourceFiles.forEach(sourceFile => visitor.visitNode(sourceFile));

visitor.missingInjectablePipes.forEach(data => {
const {classDeclaration, importDeclarationMissingImport} = data;
Expand Down
5 changes: 3 additions & 2 deletions packages/core/schematics/migrations/move-document/index.ts
View file
Expand Up @@ -54,10 +54,11 @@ function runMoveDocumentMigration(tree: Tree, tsconfigPath: string, basePath: st
const program = ts.createProgram(parsed.fileNames, parsed.options, host);
const typeChecker = program.getTypeChecker();
const visitor = new DocumentImportVisitor(typeChecker);
const rootSourceFiles = program.getRootFileNames().map(f => program.getSourceFile(f) !);
const sourceFiles = program.getSourceFiles().filter(
f => !f.isDeclarationFile && !program.isSourceFileFromExternalLibrary(f));

// Analyze source files by finding imports.
rootSourceFiles.forEach(sourceFile => visitor.visitNode(sourceFile));
sourceFiles.forEach(sourceFile => visitor.visitNode(sourceFile));

const {importsMap} = visitor;

Expand Down
3 changes: 2 additions & 1 deletion packages/core/schematics/migrations/static-queries/index.ts
View file
Expand Up @@ -117,7 +117,8 @@ function analyzeProject(tree: Tree, tsconfigPath: string, basePath: string):

const program = ts.createProgram(parsed.fileNames, parsed.options, host);
const typeChecker = program.getTypeChecker();
const sourceFiles = program.getRootFileNames().map(f => program.getSourceFile(f) !);
const sourceFiles = program.getSourceFiles().filter(
f => !f.isDeclarationFile && !program.isSourceFileFromExternalLibrary(f));
const queryVisitor = new NgQueryResolveVisitor(typeChecker);

// Analyze all project source-files and collect all queries that
Expand Down
5 changes: 3 additions & 2 deletions packages/core/schematics/migrations/template-var-assignment/index.ts
View file
Expand Up @@ -60,10 +60,11 @@ function runTemplateVariableAssignmentCheck(
const program = ts.createProgram(parsed.fileNames, parsed.options, host);
const typeChecker = program.getTypeChecker();
const templateVisitor = new NgComponentTemplateVisitor(typeChecker);
const rootSourceFiles = program.getRootFileNames().map(f => program.getSourceFile(f) !);
const sourceFiles = program.getSourceFiles().filter(
f => !f.isDeclarationFile && !program.isSourceFileFromExternalLibrary(f));

// Analyze source files by detecting HTML templates.
rootSourceFiles.forEach(sourceFile => templateVisitor.visitNode(sourceFile));
sourceFiles.forEach(sourceFile => templateVisitor.visitNode(sourceFile));

const {resolvedTemplates} = templateVisitor;
const collectedFailures: string[] = [];
Expand Down

2 comments on commit e8ceae1

@IgorMinar
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
Copy link
Member Author

@devversion devversion commented on e8ceae1 May 14, 2019 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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