Skip to content
Permalink
Browse files

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
  • Loading branch information...
alan-agius4 authored and mhevery committed May 29, 2019
1 parent 65f2010 commit 26e3615e19b8ef15d9b5b399079b0596ec2bf823
@@ -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);
@@ -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);
@@ -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);
@@ -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);
@@ -60,6 +60,20 @@ describe('injectable pipe migration', () => {
.toMatch(/@Injectable\(\)\s+@Pipe\(\{ name: 'myPipe' \}\)\s+export class MyPipe/);
});

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';
@@ -60,6 +60,19 @@ describe('move-document migration', () => {
expect(content).not.toContain(`import {DOCUMENT} from '@angular/platform-browser';`);
});

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';
@@ -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: \`
<ng-template #myTmpl>
My template
</ng-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';
@@ -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: '<span #test></span>'})
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';

0 comments on commit 26e3615

Please sign in to comment.
You can’t perform that action at this time.