From 80394ce08b12ff95167cf1c4aec61044bb78fbe6 Mon Sep 17 00:00:00 2001 From: Alan Agius Date: Wed, 29 May 2019 12:05:49 +0200 Subject: [PATCH] fix(core): TypeScript related migrations should cater for BOM (#30719) fix(@schematics/angular): TypeScript related migrations should cater for BOM In the CLI `UpdateRecorder` methods such as `insertLeft`, `remove` etc.. accepts positions which are not offset by a BOM. This is because when a file has a BOM a different recorder will be used https://github.com/angular/angular-cli/blob/master/packages/angular_devkit/schematics/src/tree/recorder.ts#L72 which caters for an addition offset/delta. The main reason for this is that when a developer is writing a schematic they shouldn't need to compute the offset based if a file has a BOM or not and is handled out of the box. Example ```ts recorder.insertLeft(5, 'true'); ``` However this is unfortunate in the case if a ts SourceFile is used and one uses `getWidth` and `getStart` method they will already be offset by 1, which at the end it results in a double offset and hence the problem. Fixes #30713 PR Close #30719 --- .../migrations/injectable-pipe/index.ts | 5 +++- .../migrations/move-document/index.ts | 5 +++- .../migrations/static-queries/index.ts | 5 +++- .../template-var-assignment/index.ts | 5 +++- .../test/injectable_pipe_migration_spec.ts | 16 ++++++++++++- .../test/move_document_migration_spec.ts | 15 +++++++++++- .../static_queries_migration_template_spec.ts | 23 +++++++++++++++++++ .../static_queries_migration_usage_spec.ts | 20 ++++++++++++++++ 8 files changed, 88 insertions(+), 6 deletions(-) diff --git a/packages/core/schematics/migrations/injectable-pipe/index.ts b/packages/core/schematics/migrations/injectable-pipe/index.ts index 5c4615bd90423..51c088677b6b7 100644 --- a/packages/core/schematics/migrations/injectable-pipe/index.ts +++ b/packages/core/schematics/migrations/injectable-pipe/index.ts @@ -47,7 +47,10 @@ function runInjectablePipeMigration(tree: Tree, tsconfigPath: string, basePath: // source files, it can end up updating query definitions multiple times. host.readFile = fileName => { const buffer = tree.read(relative(basePath, fileName)); - return buffer ? buffer.toString() : undefined; + // Strip BOM as otherwise TSC methods (Ex: getWidth) will return an offset which + // which breaks the CLI UpdateRecorder. + // See: https://github.com/angular/angular/pull/30719 + return buffer ? buffer.toString().replace(/^\uFEFF/, '') : undefined; }; const program = ts.createProgram(parsed.fileNames, parsed.options, host); diff --git a/packages/core/schematics/migrations/move-document/index.ts b/packages/core/schematics/migrations/move-document/index.ts index ffb2a5907919a..30d0cc5597ea6 100644 --- a/packages/core/schematics/migrations/move-document/index.ts +++ b/packages/core/schematics/migrations/move-document/index.ts @@ -48,7 +48,10 @@ function runMoveDocumentMigration(tree: Tree, tsconfigPath: string, basePath: st // source files, it can end up updating query definitions multiple times. host.readFile = fileName => { const buffer = tree.read(relative(basePath, fileName)); - return buffer ? buffer.toString() : undefined; + // Strip BOM as otherwise TSC methods (Ex: getWidth) will return an offset which + // which breaks the CLI UpdateRecorder. + // See: https://github.com/angular/angular/pull/30719 + return buffer ? buffer.toString().replace(/^\uFEFF/, '') : undefined; }; const program = ts.createProgram(parsed.fileNames, parsed.options, host); diff --git a/packages/core/schematics/migrations/static-queries/index.ts b/packages/core/schematics/migrations/static-queries/index.ts index a12f1c06baf58..207531bbebdd4 100644 --- a/packages/core/schematics/migrations/static-queries/index.ts +++ b/packages/core/schematics/migrations/static-queries/index.ts @@ -125,7 +125,10 @@ function analyzeProject( // source files, it can end up updating query definitions multiple times. host.readFile = fileName => { const buffer = tree.read(relative(basePath, fileName)); - return buffer ? buffer.toString() : undefined; + // Strip BOM as otherwise TSC methods (Ex: getWidth) will return an offset which + // which breaks the CLI UpdateRecorder. + // See: https://github.com/angular/angular/pull/30719 + return buffer ? buffer.toString().replace(/^\uFEFF/, '') : undefined; }; const program = ts.createProgram(parsed.fileNames, parsed.options, host); diff --git a/packages/core/schematics/migrations/template-var-assignment/index.ts b/packages/core/schematics/migrations/template-var-assignment/index.ts index e6b0ddef486bd..a7fce2081b12e 100644 --- a/packages/core/schematics/migrations/template-var-assignment/index.ts +++ b/packages/core/schematics/migrations/template-var-assignment/index.ts @@ -53,7 +53,10 @@ function runTemplateVariableAssignmentCheck( // program to be based on the file contents in the virtual file tree. host.readFile = fileName => { const buffer = tree.read(relative(basePath, fileName)); - return buffer ? buffer.toString() : undefined; + // Strip BOM as otherwise TSC methods (Ex: getWidth) will return an offset which + // which breaks the CLI UpdateRecorder. + // See: https://github.com/angular/angular/pull/30719 + return buffer ? buffer.toString().replace(/^\uFEFF/, '') : undefined; }; const program = ts.createProgram(parsed.fileNames, parsed.options, host); diff --git a/packages/core/schematics/test/injectable_pipe_migration_spec.ts b/packages/core/schematics/test/injectable_pipe_migration_spec.ts index be238ed894e56..a07732b4e9cc3 100644 --- a/packages/core/schematics/test/injectable_pipe_migration_spec.ts +++ b/packages/core/schematics/test/injectable_pipe_migration_spec.ts @@ -60,7 +60,21 @@ describe('injectable pipe migration', () => { .toMatch(/@Injectable\(\)\s+@Pipe\(\{ name: 'myPipe' \}\)\s+export class MyPipe/); }); - it('should add an import for Injectable to the @angular/core import declaration', async() => { + it('should add @Injectable to pipes that do not have it (BOM)', () => { + writeFile('/index.ts', `\uFEFF + import { Pipe } from '@angular/core'; + + @Pipe({ name: 'myPipe' }) + export class MyPipe { + } + `); + + runMigration(); + expect(tree.readContent('/index.ts')) + .toMatch(/@Injectable\(\)\s+@Pipe\(\{ name: 'myPipe' \}\)\s+export class MyPipe/); + }); + + it('should add an import for Injectable to the @angular/core import declaration', () => { writeFile('/index.ts', ` import { Pipe } from '@angular/core'; diff --git a/packages/core/schematics/test/move_document_migration_spec.ts b/packages/core/schematics/test/move_document_migration_spec.ts index f7093e678589c..066f04a122117 100644 --- a/packages/core/schematics/test/move_document_migration_spec.ts +++ b/packages/core/schematics/test/move_document_migration_spec.ts @@ -60,7 +60,20 @@ describe('move-document migration', () => { expect(content).not.toContain(`import {DOCUMENT} from '@angular/platform-browser';`); }); - it('should properly apply import replacement with existing import', async() => { + it('should properly apply import replacement (BOM)', () => { + writeFile('/index.ts', `\uFEFF + import {DOCUMENT} from '@angular/platform-browser'; + `); + + runMigration(); + + const content = tree.readContent('/index.ts'); + + expect(content).toContain(`import { DOCUMENT } from "@angular/common";`); + expect(content).not.toContain(`import {DOCUMENT} from '@angular/platform-browser';`); + }); + + it('should properly apply import replacement with existing import', () => { writeFile('/index.ts', ` import {DOCUMENT} from '@angular/platform-browser'; import {someImport} from '@angular/common'; 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 bafd77b11fb11..89956ce4dcb94 100644 --- a/packages/core/schematics/test/static_queries_migration_template_spec.ts +++ b/packages/core/schematics/test/static_queries_migration_template_spec.ts @@ -158,6 +158,29 @@ describe('static-queries migration with template strategy', () => { .toContain(`@ViewChild('myTmpl', { static: true }) query: any;`); }); + it('should detect queries selecting ng-template as static (BOM)', async() => { + writeFile('/index.ts', `\uFEFF + import {Component, NgModule, ViewChild} from '@angular/core'; + + @Component({template: \` + + My template + + \`}) + export class MyComp { + private @ViewChild('myTmpl') query: any; + } + + @NgModule({declarations: [MyComp]}) + export class MyModule {} + `); + + await runMigration(); + + expect(tree.readContent('/index.ts')) + .toContain(`@ViewChild('myTmpl', { static: true }) query: any;`); + }); + it('should detect queries selecting component view providers through string token', async() => { writeFile('/index.ts', ` import {Component, Directive, NgModule, ViewChild} from '@angular/core'; diff --git a/packages/core/schematics/test/static_queries_migration_usage_spec.ts b/packages/core/schematics/test/static_queries_migration_usage_spec.ts index c1e065276f22b..02946c884dc12 100644 --- a/packages/core/schematics/test/static_queries_migration_usage_spec.ts +++ b/packages/core/schematics/test/static_queries_migration_usage_spec.ts @@ -127,6 +127,26 @@ describe('static-queries migration with usage strategy', () => { .toContain(`@ContentChild('test', { static: false }) query: any;`); }); + it('should not mark content queries used in "ngAfterContentInit" as static (BOM)', async() => { + writeFile('/index.ts', `\uFEFF + import {Component, ContentChild} from '@angular/core'; + + @Component({template: ''}) + export class MyComp { + @ContentChild('test') query: any; + + ngAfterContentInit() { + this.query.classList.add('test'); + } + } + `); + + await runMigration(); + + expect(tree.readContent('/index.ts')) + .toContain(`@ContentChild('test', { static: false }) query: any;`); + }); + it('should not mark content queries used in "ngAfterContentChecked" as static', async() => { writeFile('/index.ts', ` import {Component, ContentChild} from '@angular/core';