From 164d160b22075f18d1b526f46db33cf79cabb72b Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Sun, 28 Apr 2019 16:40:59 +0200 Subject: [PATCH] refactor(core): static-query migrations fails if options cannot be transformed (#30178) Currently the `static-query` migrations fails at the final step of updating a query when the query already specifies options which cannot be transformed easily. e.g. the options are computed through a function call: `@ViewChild(..., getQueryOpts());` or `@ViewChild(..., myOptionsVar)`. In these cases we technically could add additionally logic to update the query options, but given that this is an edge-case and it's potentially over-engineering the migration schematic, we just always add a TODO for the timing and print out the determined query timing in the console. The developer in that case just needs to manually update the logic for the query options to contain the printed query timing. Potentially related to: https://github.com/angular/angular-cli/issues/14298 PR Close #30178 --- .../google3/explicitQueryTimingRule.ts | 6 +- .../migrations/static-queries/index.ts | 11 +-- .../migrations/static-queries/transform.ts | 80 +++++++++++++------ .../static_queries_migration_template_spec.ts | 25 ++++++ 4 files changed, 91 insertions(+), 31 deletions(-) diff --git a/packages/core/schematics/migrations/static-queries/google3/explicitQueryTimingRule.ts b/packages/core/schematics/migrations/static-queries/google3/explicitQueryTimingRule.ts index 7e4fd64c86..27af87fb73 100644 --- a/packages/core/schematics/migrations/static-queries/google3/explicitQueryTimingRule.ts +++ b/packages/core/schematics/migrations/static-queries/google3/explicitQueryTimingRule.ts @@ -64,13 +64,13 @@ export class Rule extends Rules.TypedRule { queries.forEach(q => { const queryExpr = q.decorator.node.expression; const {timing, message} = usageStrategy.detectTiming(q); - const transformedNode = getTransformedQueryCallExpr(q, timing, !!message); + const result = getTransformedQueryCallExpr(q, timing, !!message); - if (!transformedNode) { + if (!result) { return; } - const newText = printer.printNode(ts.EmitHint.Unspecified, transformedNode, sourceFile); + const newText = printer.printNode(ts.EmitHint.Unspecified, result.node, sourceFile); // Replace the existing query decorator call expression with the // updated call expression node. diff --git a/packages/core/schematics/migrations/static-queries/index.ts b/packages/core/schematics/migrations/static-queries/index.ts index 0b4797f49c..6faf0aedef 100644 --- a/packages/core/schematics/migrations/static-queries/index.ts +++ b/packages/core/schematics/migrations/static-queries/index.ts @@ -192,23 +192,24 @@ async function runStaticQueryMigration( queries.forEach(q => { const queryExpr = q.decorator.node.expression; const {timing, message} = strategy.detectTiming(q); - const transformedNode = getTransformedQueryCallExpr(q, timing, !!message); + const result = getTransformedQueryCallExpr(q, timing, !!message); - if (!transformedNode) { + if (!result) { return; } - const newText = printer.printNode(ts.EmitHint.Unspecified, transformedNode, sourceFile); + const newText = printer.printNode(ts.EmitHint.Unspecified, result.node, sourceFile); // Replace the existing query decorator call expression with the updated // call expression node. update.remove(queryExpr.getStart(), queryExpr.getWidth()); update.insertRight(queryExpr.getStart(), newText); - if (message) { + if (result.failureMessage || message) { const {line, character} = ts.getLineAndCharacterOfPosition(sourceFile, q.decorator.node.getStart()); - failureMessages.push(`${relativePath}@${line + 1}:${character + 1}: ${message}`); + failureMessages.push( + `${relativePath}@${line + 1}:${character + 1}: ${result.failureMessage || message}`); } }); diff --git a/packages/core/schematics/migrations/static-queries/transform.ts b/packages/core/schematics/migrations/static-queries/transform.ts index 1139afcdc8..14aec6ca2e 100644 --- a/packages/core/schematics/migrations/static-queries/transform.ts +++ b/packages/core/schematics/migrations/static-queries/transform.ts @@ -10,6 +10,13 @@ import * as ts from 'typescript'; import {getPropertyNameText} from '../../utils/typescript/property_name'; import {NgQueryDefinition, QueryTiming} from './angular/query-definition'; +export type TransformedQueryResult = null | { + /** Transformed call expression. */ + node: ts.CallExpression; + /** Failure message which is set when the query could not be transformed successfully. */ + failureMessage: string|null; +}; + const TODO_COMMENT = 'TODO: add static flag'; /** @@ -17,8 +24,8 @@ const TODO_COMMENT = 'TODO: add static flag'; * determined timing. The updated decorator call expression node will be returned. */ export function getTransformedQueryCallExpr( - query: NgQueryDefinition, timing: QueryTiming | null, createTodo: boolean): ts.CallExpression| - null { + query: NgQueryDefinition, timing: QueryTiming | null, + createTodo: boolean): TransformedQueryResult { const queryExpr = query.decorator.node.expression; const queryArguments = queryExpr.arguments; const queryPropertyAssignments = timing === null ? @@ -27,29 +34,52 @@ export function getTransformedQueryCallExpr( 'static', timing === QueryTiming.STATIC ? ts.createTrue() : ts.createFalse())]; // If the query decorator is already called with two arguments, we need to - // keep the existing options untouched and just add the new property if needed. + // keep the existing options untouched and just add the new property if possible. if (queryArguments.length === 2) { - const existingOptions = queryArguments[1] as ts.ObjectLiteralExpression; + const existingOptions = queryArguments[1]; + const hasTodoComment = existingOptions.getFullText().includes(TODO_COMMENT); + let newOptionsNode: ts.Expression; + let failureMessage: string|null = null; - // In case the options already contains a property for the "static" flag, we just - // skip this query and leave it untouched. - if (existingOptions.properties.some( - p => !!p.name && getPropertyNameText(p.name) === 'static')) { - return null; + if (ts.isObjectLiteralExpression(existingOptions)) { + // In case the options already contains a property for the "static" flag, + // we just skip this query and leave it untouched. + if (existingOptions.properties.some( + p => !!p.name && getPropertyNameText(p.name) === 'static')) { + return null; + } + + newOptionsNode = ts.updateObjectLiteral( + existingOptions, existingOptions.properties.concat(queryPropertyAssignments)); + + // In case we want to add a todo and the options do not have the todo + // yet, we add the query timing todo as synthetic multi-line comment. + if (createTodo && !hasTodoComment) { + addQueryTimingTodoToNode(newOptionsNode); + } + } else { + // In case the options query parameter is not an object literal expression, and + // we want to set the query timing, we just preserve the existing query parameter. + newOptionsNode = existingOptions; + // We always want to add a TODO in case the query options cannot be updated. + if (!hasTodoComment) { + addQueryTimingTodoToNode(existingOptions); + } + // If there is a new explicit timing that has been determined for the given query, + // we create a transformation failure message that shows developers that they need + // to set the query timing manually to the determined query timing. + if (timing !== null) { + failureMessage = 'Cannot update query declaration to explicit timing. Please manually ' + + `set the query timing to: "{static: ${(timing === QueryTiming.STATIC).toString()}}"`; + } } - const updatedOptions = ts.updateObjectLiteral( - existingOptions, existingOptions.properties.concat(queryPropertyAssignments)); - - // In case we want to add a todo and the options do not have the todo - // yet, we add the query timing todo as synthetic multi-line comment. - if (createTodo && !existingOptions.getFullText().includes(TODO_COMMENT)) { - addQueryTimingTodoToNode(updatedOptions); - } - - return ts.updateCall( - queryExpr, queryExpr.expression, queryExpr.typeArguments, - [queryArguments[0], updatedOptions]); + return { + failureMessage, + node: ts.updateCall( + queryExpr, queryExpr.expression, queryExpr.typeArguments, + [queryArguments[0], newOptionsNode !]) + }; } const optionsNode = ts.createObjectLiteral(queryPropertyAssignments); @@ -58,8 +88,12 @@ export function getTransformedQueryCallExpr( addQueryTimingTodoToNode(optionsNode); } - return ts.updateCall( - queryExpr, queryExpr.expression, queryExpr.typeArguments, [queryArguments[0], optionsNode]); + return { + failureMessage: null, + node: ts.updateCall( + queryExpr, queryExpr.expression, queryExpr.typeArguments, + [queryArguments[0], optionsNode]) + }; } /** diff --git a/packages/core/schematics/test/static_queries_migration_template_spec.ts b/packages/core/schematics/test/static_queries_migration_template_spec.ts index 5e8ef131f3..e9ae5ad0a3 100644 --- a/packages/core/schematics/test/static_queries_migration_template_spec.ts +++ b/packages/core/schematics/test/static_queries_migration_template_spec.ts @@ -492,6 +492,31 @@ describe('static-queries migration with template strategy', () => { .toMatch(/^⮑ {3}index.ts@6:11: Content queries cannot be migrated automatically\./); }); + it('should add a todo if query options cannot be migrated inline', async() => { + writeFile('/index.ts', ` + import {Component, NgModule, ViewChild} from '@angular/core'; + + const myOptionsVar = {}; + + @Component({template: '

'}) + export class MyComp { + @ViewChild('myRef', myOptionsVar) query: any; + } + + @NgModule({declarations: [MyComp]}) + export class MyModule {} + `); + + await runMigration(); + + expect(tree.readContent('/index.ts')) + .toContain(`@ViewChild('myRef', /* TODO: add static flag */ myOptionsVar) query: any;`); + expect(warnOutput.length).toBe(1); + expect(warnOutput[0]) + .toMatch(/^⮑ {3}index.ts@8:11: Cannot update query declaration to explicit timing./); + expect(warnOutput[0]).toMatch(/Please manually set the query timing to.*static: true/); + }); + it('should not normalize stylesheets which are referenced in component', async() => { writeFile('sub_dir/index.ts', ` import {Component, NgModule, ContentChild} from '@angular/core';