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

Migrating to v8 changes ViewChild to VViewChild #30713

Closed
dushkostanoeski opened this issue May 29, 2019 · 6 comments

Comments

@dushkostanoeski
Copy link

commented May 29, 2019

Description

This might be connected to angular/angular-cli#14551 and angular/angular-cli#9712

After I ran the migration, all my ViewChild references turned into VViewChild.

Edit: I just noticed that this only happened to the files with UTF-8 encoding. The files encoded with UTF-8 BOM are good.

@alan-agius4 alan-agius4 self-assigned this May 29, 2019

@ngbot ngbot bot added this to the needsTriage milestone May 29, 2019

@devversion

This comment has been minimized.

Copy link
Member

commented May 29, 2019

@alan-agius4 Is this is actually an issue from the schematic? My impression is that this is related to the update recorder from the CLI that every migration schematic leverages.

That's why similar issues appear for the lazy route syntax migration. I think we should have a more generic issue for this on the CLI repository as it seems to affect all migration schematics. Also the ones in the CLI.

@devversion devversion removed the comp: core label May 29, 2019

@ngbot ngbot bot removed this from the needsTriage milestone May 29, 2019

@alan-agius4

This comment has been minimized.

Copy link
Contributor

commented May 29, 2019

As discussed offline, the problem/feature is indeed in the CLI because the 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.

alan-agius4 added a commit to alan-agius4/angular that referenced this issue 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 angular#30713

alan-agius4 added a commit to alan-agius4/angular that referenced this issue 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 angular#30713
@devversion

This comment has been minimized.

Copy link
Member

commented May 29, 2019

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.

Just to add to that: I see the point of doing that, but it is simply resulting in magic behavior as schematic indices are not determined by just looking at the visible rendered characters but rather by parsed source-files where the BOM is a valid invisible character that still adds to the amount characters.

e.g. fs.readFileSync('./with_bom.txt', 'utf8').length will respect the BOM characters and therefore it's common that indices passed to the update recorder will be truly absolute.

Ideally the magic differentation between BOM and non-BOM files would have not happened at all, but for now the easiest way to fix this in a backwards-compatible way (where older CLI versions are used) is to strip the BOM when parsing the TypeScript source-files.

We need to find a proper solution for this in the future though. It adds unnecessary bloat and confusion to schematic authors as they need to know about different behavior with BOM / and need to handle BOM characters in a special way for now.

@alan-agius4

This comment has been minimized.

Copy link
Contributor

commented May 29, 2019

@devversion I think this discussion should be moved in a new issue in the CLI repo.

@devversion

This comment has been minimized.

Copy link
Member

commented May 29, 2019

@alan-agius4 Yeah I was asking for that one in a post above. I'm working on creating one 🙂

@devversion

This comment has been minimized.

Copy link
Member

commented May 29, 2019

Thanks @dushkostanoeski for submitting that issue!

A temporary fix for this has been created in #30719, but the underlying issue is tracked with angular/angular-cli#14558. We can close this issue once the temporary fix landed.

@matsko matsko added the comp: core label May 30, 2019

@ngbot ngbot bot added this to the needsTriage milestone May 30, 2019

@ngbot ngbot bot modified the milestones: needsTriage, Backlog May 30, 2019

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

alan-agius4 added a commit to alan-agius4/angular that referenced this issue Jun 6, 2019

fix(core): 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 angular#30713

alan-agius4 added a commit to alan-agius4/angular that referenced this issue Jun 6, 2019

fix(core): 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 angular#30713

mhevery added a commit that referenced this issue 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.