Skip to content

refactor(core): static-query migration should always use template strategy #30628

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
devversion wants to merge 2 commits into from
Closed

refactor(core): static-query migration should always use template strategy #30628

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
45f558e
refactor(core): static-query migration should always use template str…
devversion May 22, 2019
40c350f
refactor(core): static-query migration should warn about syntax failures
devversion May 23, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 31 additions & 10 deletions packages/core/schematics/migrations/static-queries/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,14 @@ import {QueryTemplateStrategy} from './strategies/template_strategy/template_str
import {QueryTestStrategy} from './strategies/test_strategy/test_strategy';
import {TimingStrategy} from './strategies/timing-strategy';
import {QueryUsageStrategy} from './strategies/usage_strategy/usage_strategy';
import {SELECTED_STRATEGY, promptForMigrationStrategy} from './strategy_prompt';
import {getTransformedQueryCallExpr} from './transform';

enum SELECTED_STRATEGY {
TEMPLATE,
USAGE,
TESTS,
}

interface AnalyzedProject {
program: ts.Program;
host: ts.CompilerHost;
Expand All @@ -50,8 +55,9 @@ async function runMigration(tree: Tree, context: SchematicContext) {
const logger = context.logger;

logger.info('------ Static Query migration ------');
logger.info('In preparation for Ivy, developers can now explicitly specify the');
logger.info('timing of their queries. Read more about this here:');
logger.info('With Angular version 8, developers need to');
logger.info('explicitly specify the timing of ViewChild or');
logger.info('ContentChild queries. Read more about this here:');
logger.info('https://github.com/angular/angular/pull/28810');
Comment thread
devversion marked this conversation as resolved.
Resolved

if (!buildPaths.length && !testPaths.length) {
Expand All @@ -63,18 +69,18 @@ async function runMigration(tree: Tree, context: SchematicContext) {
const analyzedFiles = new Set<string>();
const buildProjects = new Set<AnalyzedProject>();
const failures = [];
const strategy = process.env['NG_STATIC_QUERY_USAGE_STRATEGY'] === 'true' ?
SELECTED_STRATEGY.USAGE :
SELECTED_STRATEGY.TEMPLATE;

for (const tsconfigPath of buildPaths) {
const project = analyzeProject(tree, tsconfigPath, basePath, analyzedFiles);
const project = analyzeProject(tree, tsconfigPath, basePath, analyzedFiles, logger);
if (project) {
buildProjects.add(project);
}
}

// In case there are projects which contain queries that need to be migrated,
// we want to prompt for the migration strategy and run the migration.
if (buildProjects.size) {
const strategy = await promptForMigrationStrategy(logger);
for (let project of Array.from(buildProjects.values())) {
failures.push(...await runStaticQueryMigration(tree, project, strategy, logger));
}
Expand All @@ -83,7 +89,7 @@ async function runMigration(tree: Tree, context: SchematicContext) {
// For the "test" tsconfig projects we always want to use the test strategy as
// we can't detect the proper timing within spec files.
for (const tsconfigPath of testPaths) {
const project = await analyzeProject(tree, tsconfigPath, basePath, analyzedFiles);
const project = await analyzeProject(tree, tsconfigPath, basePath, analyzedFiles, logger);
if (project) {
failures.push(
...await runStaticQueryMigration(tree, project, SELECTED_STRATEGY.TESTS, logger));
Expand All @@ -105,7 +111,8 @@ async function runMigration(tree: Tree, context: SchematicContext) {
* migrated. In case there are no queries that can be migrated, null is returned.
*/
function analyzeProject(
tree: Tree, tsconfigPath: string, basePath: string, analyzedFiles: Set<string>):
tree: Tree, tsconfigPath: string, basePath: string, analyzedFiles: Set<string>,
logger: logging.LoggerApi):
AnalyzedProject|null {
const parsed = parseTsconfigFile(tsconfigPath, dirname(tsconfigPath));
const host = ts.createCompilerHost(parsed.options, true);
Expand All @@ -120,6 +127,20 @@ function analyzeProject(
};

const program = ts.createProgram(parsed.fileNames, parsed.options, host);
const syntacticDiagnostics = program.getSyntacticDiagnostics();

// Syntactic TypeScript errors can throw off the query analysis and therefore we want
// to notify the developer that we couldn't analyze parts of the project. Developers
// can just re-run the migration after fixing these failures.
if (syntacticDiagnostics.length) {
logger.warn(
`\nTypeScript project "${tsconfigPath}" has syntactical errors which could cause ` +
`an incomplete migration. Please fix the following failures and rerun the migration:`);
logger.error(ts.formatDiagnostics(syntacticDiagnostics, host));
logger.info(
'Migration can be rerun with: "ng update @angular/core --from 7 --to 8 --migrate-only"\n');
}

const typeChecker = program.getTypeChecker();
const sourceFiles = program.getSourceFiles().filter(
f => !f.isDeclarationFile && !program.isSourceFileFromExternalLibrary(f));
Expand Down Expand Up @@ -192,7 +213,7 @@ async function runStaticQueryMigration(
} catch (e) {
if (selectedStrategy === SELECTED_STRATEGY.TEMPLATE) {
logger.warn(
`The template migration strategy uses the Angular compiler ` +
`\nThe template migration strategy uses the Angular compiler ` +
`internally and therefore projects that no longer build successfully after ` +
`the update cannot use the template migration strategy. Please ensure ` +
`there are no AOT compilation errors.\n`);
Expand Down
14 changes: 5 additions & 9 deletions ...re/schematics/migrations/static-queries/strategies/template_strategy/template_strategy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,13 +57,9 @@ export class QueryTemplateStrategy implements TimingStrategy {
// used to determine the timing for registered queries.
const analyzedModules = (aotProgram as any)['analyzedModules'] as NgAnalyzedModules;

const ngDiagnostics = [
...aotProgram.getNgStructuralDiagnostics(),
...aotProgram.getNgSemanticDiagnostics(),
];

if (ngDiagnostics.length) {
throw this._createDiagnosticsError(ngDiagnostics);
const ngStructuralDiagnostics = aotProgram.getNgStructuralDiagnostics();
if (ngStructuralDiagnostics.length) {
throw this._createDiagnosticsError(ngStructuralDiagnostics);
}

analyzedModules.files.forEach(file => {
Expand Down Expand Up @@ -178,8 +174,8 @@ export class QueryTemplateStrategy implements TimingStrategy {
.template;
}

private _createDiagnosticsError(diagnostics: (ts.Diagnostic|Diagnostic)[]) {
return new Error(`${diagnostics.map(d => d.messageText).join(`\n `)}`);
private _createDiagnosticsError(diagnostics: ReadonlyArray<Diagnostic>) {
return new Error(ts.formatDiagnostics(diagnostics as ts.Diagnostic[], this.host));
}

private _getViewQueryUniqueKey(filePath: string, className: string, propName: string) {
Expand Down
50 changes: 0 additions & 50 deletions packages/core/schematics/migrations/static-queries/strategy_prompt.ts
View file
Open in desktop

This file was deleted.

109 changes: 109 additions & 0 deletions packages/core/schematics/test/static_queries_migration_template_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -498,6 +498,115 @@ describe('static-queries migration with template strategy', () => {
/^⮑ {3}index.ts@5:11: Multiple components use the query with different timings./);
});

it('should be able to migrate an application with type checking failure which ' +
'does not affect analysis',
async() => {
// Fakes the `@angular/package` by creating a `ViewChild` decorator
// function that requires developers to specify the "static" flag.
writeFile('/node_modules/@angular/core/index.d.ts', `
export interface ViewChildDecorator {
(selector: Type<any> | Function | string, opts: {
static: boolean;
read?: any;
}): any;
}

export declare const ViewChild: ViewChildDecorator;
`);

writeFile('/index.ts', `
import {NgModule, Component, ViewChild} from '@angular/core';

@Component({
template: '<ng-template><p #myRef></p></ng-template>'
})
export class MyComp {
@ViewChild('myRef') query: any;
}
`);

writeFile('/my-module.ts', `
import {NgModule} from '@angular/core';
import {MyComp} from './index';

@NgModule({declarations: [MyComp]})
export class MyModule {}
`);

await runMigration();

expect(errorOutput.length).toBe(0);
expect(tree.readContent('/index.ts'))
.toContain(`@ViewChild('myRef', { static: false }) query: any;`);
});

it('should be able to migrate applications with template type checking failure ' +
'which does not affect analysis',
async() => {
writeFile('/index.ts', `
import {NgModule, Component, ViewChild} from '@angular/core';

@Component({
template: '<p #myRef>{{myVar.hello()}}</p>'
})
export class MyComp {
// This causes a type checking exception as the template
// tries to call a function called "hello()" on this variable.
myVar: boolean = false;

@ViewChild('myRef') query: any;
}
`);

writeFile('/my-module.ts', `
import {NgModule} from '@angular/core';
import {MyComp} from './index';

@NgModule({declarations: [MyComp]})
export class MyModule {}
`);

await runMigration();

expect(errorOutput.length).toBe(0);
expect(tree.readContent('/index.ts'))
.toContain(`@ViewChild('myRef', { static: true }) query: any;`);
});

it('should notify user if project has syntax errors which can affect analysis', async() => {
writeFile('/index.ts', `
import {Component, ViewChild} from '@angular/core';

@Component({
template: '<p #myRef></p>'
})
export class MyComp {
@ViewChild('myRef') query: any;
}
`);

writeFile('/file-with-syntax-error.ts', `
export classX ClassWithSyntaxError {
// ...
}
`);

writeFile('/my-module.ts', `
import {NgModule} from '@angular/core';
import {MyComp} from './index';

@NgModule({declarations: [MyComp]})
export class MyModule {}
`);

await runMigration();

expect(errorOutput.length).toBe(1);
expect(errorOutput[0]).toMatch(/file-with-syntax-error\.ts\(2,9\): error TS1128.*/);
expect(tree.readContent('/index.ts'))
.toContain(`@ViewChild('myRef', { static: true }) query: any;`);
});

it('should gracefully exit migration if queries could not be analyzed', async() => {
writeFile('/index.ts', `
import {Component, ViewChild} from '@angular/core';
Expand Down
17 changes: 0 additions & 17 deletions packages/core/schematics/test/static_queries_migration_usage_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1506,23 +1506,6 @@ describe('static-queries migration with usage strategy', () => {
.toContain(`@${queryType}('test', { static: false }) query: any;`);
});

it(`should not prompt for migration strategy if no @${queryType} query is used`, async() => {
writeFile('/index.ts', `
import {Component, ${queryType}} from '@angular/core';

@Component({template: '<span #test></span>'})
export class NoQueriesDeclared {
}
`);

const testModule = require('../migrations/static-queries/strategy_prompt');
spyOn(testModule, 'promptForMigrationStrategy').and.callThrough();

await runMigration();

expect(testModule.promptForMigrationStrategy).toHaveBeenCalledTimes(0);
});

it('should support function call with default parameter value', async() => {
writeFile('/index.ts', `
import {Component, ${queryType}} from '@angular/core';
Expand Down