From 4a2fb86b1ea05376d428d594d9ab738579e9bec0 Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Thu, 4 Apr 2019 12:52:29 +0200 Subject: [PATCH] refactor(core): polish failure messages for template-var-assignment schematic (#29708) Improves the failure messages for the `template-var-assignment` schematic. After manual testing of the schematic it's not quite clear for developers what the failure message means without any context. The schematic now also references a short markdown file mentioning what needs to be changed, but eventually this document needs to be expanded with more information and context of the reasoning behind this change within Ivy. PR Close #29708 --- .../template-var-assignment/README.md | 27 ++++++++++++++ .../template-var-assignment/index.ts | 20 +++++++++-- .../template_var_assignment_migration_spec.ts | 35 +++++++++++-------- 3 files changed, 64 insertions(+), 18 deletions(-) create mode 100644 packages/core/schematics/migrations/template-var-assignment/README.md diff --git a/packages/core/schematics/migrations/template-var-assignment/README.md b/packages/core/schematics/migrations/template-var-assignment/README.md new file mode 100644 index 0000000000..2cf411fec7 --- /dev/null +++ b/packages/core/schematics/migrations/template-var-assignment/README.md @@ -0,0 +1,27 @@ +## Assignments to template variables + +With Ivy, assignments to template variables are no longer supported +as template variables are effectively constants. + +This means that assignments to template variables will break your +application once Ivy is enabled by default. For example: + +```html + +``` + +In the example from above, a value is assigned to the `option` +template variable on `click`. This will ultimately break your +application and therefore the logic needs to be adjusted to not +update the `option` variable, but rather the given element in +the `options` array: + +```html + +``` \ No newline at end of file diff --git a/packages/core/schematics/migrations/template-var-assignment/index.ts b/packages/core/schematics/migrations/template-var-assignment/index.ts index 2adc588d11..d8c520a30a 100644 --- a/packages/core/schematics/migrations/template-var-assignment/index.ts +++ b/packages/core/schematics/migrations/template-var-assignment/index.ts @@ -19,6 +19,10 @@ import {NgComponentTemplateVisitor} from './angular/ng_component_template'; type Logger = logging.LoggerApi; +const README_URL = + 'https://github.com/angular/angular/tree/master/packages/core/schematics/migrations/template-var-assignment/README.md'; +const FAILURE_MESSAGE = `Found assignment to template variable.`; + /** Entry point for the V8 template variable assignment schematic. */ export default function(): Rule { return (tree: Tree, context: SchematicContext) => { @@ -62,6 +66,7 @@ function runTemplateVariableAssignmentCheck( rootSourceFiles.forEach(sourceFile => templateVisitor.visitNode(sourceFile)); const {resolvedTemplates} = templateVisitor; + const collectedFailures: string[] = []; // Analyze each resolved template and print a warning for property writes to // template variables. @@ -76,9 +81,18 @@ function runTemplateVariableAssignmentCheck( nodes.forEach(n => { const {line, character} = template.getCharacterAndLineOfPosition(n.start); - logger.warn( - `${displayFilePath}@${line + 1}:${character + 1}: Found assignment to template ` + - `variable. This does not work with Ivy and needs to be updated.`); + collectedFailures.push(`${displayFilePath}@${line + 1}:${character + 1}: ${FAILURE_MESSAGE}`); }); }); + + if (collectedFailures.length) { + logger.info('---- Template Variable Assignment schematic ----'); + logger.info('Assignments to template variables will no longer work with Ivy as'); + logger.info('template variables are effectively constants in Ivy. Read more about'); + logger.info(`this change here: ${README_URL}`); + logger.info(''); + logger.info('The following template assignments were found:'); + collectedFailures.forEach(failure => logger.warn(`⮑ ${failure}`)); + logger.info('------------------------------------------------'); + } } diff --git a/packages/core/schematics/test/template_var_assignment_migration_spec.ts b/packages/core/schematics/test/template_var_assignment_migration_spec.ts index 918459a2da..2b56dd5c41 100644 --- a/packages/core/schematics/test/template_var_assignment_migration_spec.ts +++ b/packages/core/schematics/test/template_var_assignment_migration_spec.ts @@ -18,7 +18,7 @@ describe('template variable assignment migration', () => { let tree: UnitTestTree; let tmpDirPath: string; let previousWorkingDir: string; - let consoleOutput: string[]; + let warnOutput: string[]; beforeEach(() => { runner = new SchematicTestRunner('test', require.resolve('../migrations.json')); @@ -31,8 +31,12 @@ describe('template variable assignment migration', () => { } })); - consoleOutput = []; - runner.logger.subscribe(logEntry => consoleOutput.push(logEntry.message)); + warnOutput = []; + runner.logger.subscribe(logEntry => { + if (logEntry.level === 'warn') { + warnOutput.push(logEntry.message); + } + }); previousWorkingDir = shx.pwd(); tmpDirPath = getSystemPath(host.root); @@ -67,8 +71,8 @@ describe('template variable assignment migration', () => { runMigration(); - expect(consoleOutput.length).toBe(1); - expect(consoleOutput[0]).toMatch(/^index.ts@5:69: Found assignment/); + expect(warnOutput.length).toBe(1); + expect(warnOutput[0]).toMatch(/^⮑ {3}index.ts@5:69: Found assignment/); }); it('should warn for two-way data binding assigning to "as" variable', () => { @@ -89,8 +93,8 @@ describe('template variable assignment migration', () => { runMigration(); - expect(consoleOutput.length).toBe(1); - expect(consoleOutput).toMatch(/^tmpl.html@3:31: Found assignment/); + expect(warnOutput.length).toBe(1); + expect(warnOutput).toMatch(/^⮑ {3}tmpl.html@3:31: Found assignment/); }); it('should warn for bound event assignments to "as" variable', () => { @@ -112,9 +116,9 @@ describe('template variable assignment migration', () => { runMigration(); - expect(consoleOutput.length).toBe(2); - expect(consoleOutput[0]).toMatch(/^sub_dir\/tmpl.html@3:25: Found assignment/); - expect(consoleOutput[1]).toMatch(/^sub_dir\/tmpl.html@4:25: Found assignment/); + expect(warnOutput.length).toBe(2); + expect(warnOutput[0]).toMatch(/^⮑ {3}sub_dir\/tmpl.html@3:25: Found assignment/); + expect(warnOutput[1]).toMatch(/^⮑ {3}sub_dir\/tmpl.html@4:25: Found assignment/); }); it('should warn for bound event assignments to template "let" variables', () => { @@ -136,9 +140,9 @@ describe('template variable assignment migration', () => { runMigration(); - expect(consoleOutput.length).toBe(2); - expect(consoleOutput[0]).toMatch(/^sub_dir\/tmpl.html@3:25: Found assignment/); - expect(consoleOutput[1]).toMatch(/^sub_dir\/tmpl.html@4:25: Found assignment/); + expect(warnOutput.length).toBe(2); + expect(warnOutput[0]).toMatch(/^⮑ {3}sub_dir\/tmpl.html@3:25: Found assignment/); + expect(warnOutput[1]).toMatch(/^⮑ {3}sub_dir\/tmpl.html@4:25: Found assignment/); }); it('should not warn for bound event assignments to component property', () => { @@ -155,7 +159,8 @@ describe('template variable assignment migration', () => { runMigration(); - expect(consoleOutput.length).toBe(0); + expect(warnOutput.length).toBe(0); + }); }); it('should not throw an error if a detected template fails parsing', () => { @@ -172,6 +177,6 @@ describe('template variable assignment migration', () => { runMigration(); - expect(consoleOutput.length).toBe(0); + expect(warnOutput.length).toBe(0); }); });