Skip to content
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.

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

fix(core): TypeScript related migrations should cater for BOM #30719

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
4 participants
@alan-agius4
Copy link
Contributor

commented May 29, 2019

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

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

//cc @devversion & @clydin

@alan-agius4 alan-agius4 requested a review from angular/fw-core as a code owner May 29, 2019

@googlebot googlebot added the cla: yes label May 29, 2019

fix(core): TypeScript related migrations should cater for BOM
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

@alan-agius4 alan-agius4 force-pushed the alan-agius4:migration-bom branch from d0dd8d2 to c06356a May 29, 2019

@devversion
Copy link
Member

left a comment

LGTM. Can you please add some small comments that explain what we replace here?

We also need a more generic issue for this on the CLI repository (so that we can link to it). Added some personal thoughts to #30713 (comment)

@alan-agius4

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2019

@devversion comments added.

@mhevery mhevery closed this in 80394ce May 31, 2019

@alan-agius4 alan-agius4 deleted the alan-agius4:migration-bom branch May 31, 2019

mhevery added a commit that referenced this pull request Jun 6, 2019

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.