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(ngcc): emit correct ES5 code #33514

Closed
wants to merge 3 commits into from

Conversation

@gkalpak
Copy link
Member

gkalpak commented Oct 31, 2019

The PR includes the following commits (see individual commit messages for more info):

  • refactor(compiler-cli): avoid superfluous parenthesis around statements
  • fix(ngcc): generate correct metadata for classes with getter/setter properties
  • fix(ngcc): do not emit ES2015 code in ES5 files

Fixes #30569, fixes #32665.

@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Oct 31, 2019

@gkalpak gkalpak force-pushed the gkalpak:fix-ngcc-es5-code branch from 21d95e8 to c567e1d Oct 31, 2019
gkalpak added a commit to gkalpak/ngcc-validation that referenced this pull request Oct 31, 2019
@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Oct 31, 2019

gkalpak added a commit to gkalpak/ngcc-validation that referenced this pull request Oct 31, 2019
gkalpak added a commit to gkalpak/ngcc-validation that referenced this pull request Oct 31, 2019
@gkalpak

This comment has been minimized.

Copy link
Member Author

gkalpak commented Oct 31, 2019

FWIW, I also tried it on the ngcc-validation repo and nothing broke.

@gkalpak gkalpak marked this pull request as ready for review Oct 31, 2019
@gkalpak gkalpak requested review from angular/fw-compiler as code owners Oct 31, 2019
@gkalpak gkalpak force-pushed the gkalpak:fix-ngcc-es5-code branch from c567e1d to e53463b Nov 4, 2019
@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Nov 4, 2019

@gkalpak gkalpak force-pushed the gkalpak:fix-ngcc-es5-code branch from e53463b to 543c80d Nov 4, 2019
@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Nov 4, 2019

You can preview 543c80d at https://pr33514-543c80d.ngbuilds.io/.

@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Nov 7, 2019

@gkalpak gkalpak force-pushed the gkalpak:fix-ngcc-es5-code branch from 90cc0e5 to b881e33 Nov 7, 2019
@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Nov 7, 2019

@alxhub
alxhub approved these changes Nov 12, 2019
@ngbot

This comment has been minimized.

Copy link

ngbot bot commented Nov 13, 2019

I see that you just added the PR action: merge label, but the following checks are still failing:
    failure conflicts with base branch "master"
    pending status "google3" is pending

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@kara

This comment has been minimized.

Copy link
Contributor

kara commented Nov 13, 2019

@gkalpak Looks like this needs a rebase

@gkalpak gkalpak force-pushed the gkalpak:fix-ngcc-es5-code branch from b881e33 to b507a50 Nov 13, 2019
@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Nov 13, 2019

@gkalpak

This comment has been minimized.

Copy link
Member Author

gkalpak commented Nov 13, 2019

@kara, rebased.

@kara

This comment has been minimized.

Copy link
Contributor

kara commented Nov 13, 2019

@gkalpak Needs one more rebase, sorry!

gkalpak added 3 commits Oct 30, 2019
Previously, due to a bug a `Context` with `isStatement: false` could be
returned in places where a `Context` with `isStatement: true` was
requested. As a result, some statements would be unnecessarily wrapped
in parenthesis.

This commit fixes the bug in `Context#withStatementMode` to always
return a `Context` with the correct `isStatement` value. Note that this
does not have any impact on the generated code other than avoiding some
superfluous parenthesis on certain statements.
…roperties

While processing class metadata, ngtsc generates a `setClassMetadata()`
call which (among other things) contains info about property decorators.
Previously, processing getter/setter pairs with some of ngcc's
`ReflectionHost`s resulted in multiple metadata entries for the same
property, which resulted in duplicate object keys, which in turn causes
an error in ES5 strict mode.

This commit fixes it by ensuring that there are no duplicate property
names in the `setClassMetadata()` calls.

In addition, `generateSetClassMetadataCall()` is updated to treat
`ClassMember#decorators: []` the same as `ClassMember.decorators: null`
(i.e. omitting the `ClassMember` from the generated `setClassMetadata()`
call). Alternatively, ngcc's `ReflectionHost`s could be updated to do
this transformation (`decorators: []` --> `decorators: null`) when
reflecting on class members, but this would require changes in many
places and be less future-proof.

For example, given a class such as:

```ts
class Foo {
  @input() get bar() { return 'bar'; }
  set bar(value: any) {}
}
```

...previously the generated `setClassMetadata()` call would look like:

```ts
ɵsetClassMetadata(..., {
  bar: [{type: Input}],
  bar: [],
});
```

The same class will now result in a call like:

```ts
ɵsetClassMetadata(..., {
  bar: [{type: Input}],
});
```

Fixes #30569
Previously, ngcc's `Renderer` would add some constants in the processed
files which were emitted as ES2015 code (e.g. `const` declarations).
This would result in invalid ES5 generated code that would break when
run on browsers that do not support the emitted format.

This commit fixes it by adding a `printStatement()` method to
`RenderingFormatter`, which can convert statements to JavaScript code in
a suitable format for the corresponding `RenderingFormatter`.
Additionally, the `translateExpression()` and `translateStatement()`
ngtsc helper methods are augmented to accept an extra hint to know
whether the code needs to be translated to ES5 format or not.

Fixes #32665
@gkalpak gkalpak force-pushed the gkalpak:fix-ngcc-es5-code branch from b507a50 to 1993cce Nov 13, 2019
@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Nov 13, 2019

@kara

This comment has been minimized.

Copy link
Contributor

kara commented Nov 13, 2019

kara added a commit that referenced this pull request Nov 13, 2019
…ts (#33514)

Previously, due to a bug a `Context` with `isStatement: false` could be
returned in places where a `Context` with `isStatement: true` was
requested. As a result, some statements would be unnecessarily wrapped
in parenthesis.

This commit fixes the bug in `Context#withStatementMode` to always
return a `Context` with the correct `isStatement` value. Note that this
does not have any impact on the generated code other than avoiding some
superfluous parenthesis on certain statements.

PR Close #33514
kara added a commit that referenced this pull request Nov 13, 2019
…roperties (#33514)

While processing class metadata, ngtsc generates a `setClassMetadata()`
call which (among other things) contains info about property decorators.
Previously, processing getter/setter pairs with some of ngcc's
`ReflectionHost`s resulted in multiple metadata entries for the same
property, which resulted in duplicate object keys, which in turn causes
an error in ES5 strict mode.

This commit fixes it by ensuring that there are no duplicate property
names in the `setClassMetadata()` calls.

In addition, `generateSetClassMetadataCall()` is updated to treat
`ClassMember#decorators: []` the same as `ClassMember.decorators: null`
(i.e. omitting the `ClassMember` from the generated `setClassMetadata()`
call). Alternatively, ngcc's `ReflectionHost`s could be updated to do
this transformation (`decorators: []` --> `decorators: null`) when
reflecting on class members, but this would require changes in many
places and be less future-proof.

For example, given a class such as:

```ts
class Foo {
  @input() get bar() { return 'bar'; }
  set bar(value: any) {}
}
```

...previously the generated `setClassMetadata()` call would look like:

```ts
ɵsetClassMetadata(..., {
  bar: [{type: Input}],
  bar: [],
});
```

The same class will now result in a call like:

```ts
ɵsetClassMetadata(..., {
  bar: [{type: Input}],
});
```

Fixes #30569

PR Close #33514
kara added a commit that referenced this pull request Nov 13, 2019
Previously, ngcc's `Renderer` would add some constants in the processed
files which were emitted as ES2015 code (e.g. `const` declarations).
This would result in invalid ES5 generated code that would break when
run on browsers that do not support the emitted format.

This commit fixes it by adding a `printStatement()` method to
`RenderingFormatter`, which can convert statements to JavaScript code in
a suitable format for the corresponding `RenderingFormatter`.
Additionally, the `translateExpression()` and `translateStatement()`
ngtsc helper methods are augmented to accept an extra hint to know
whether the code needs to be translated to ES5 format or not.

Fixes #32665

PR Close #33514
@kara kara closed this in c79d50f Nov 13, 2019
kara added a commit that referenced this pull request Nov 13, 2019
…roperties (#33514)

While processing class metadata, ngtsc generates a `setClassMetadata()`
call which (among other things) contains info about property decorators.
Previously, processing getter/setter pairs with some of ngcc's
`ReflectionHost`s resulted in multiple metadata entries for the same
property, which resulted in duplicate object keys, which in turn causes
an error in ES5 strict mode.

This commit fixes it by ensuring that there are no duplicate property
names in the `setClassMetadata()` calls.

In addition, `generateSetClassMetadataCall()` is updated to treat
`ClassMember#decorators: []` the same as `ClassMember.decorators: null`
(i.e. omitting the `ClassMember` from the generated `setClassMetadata()`
call). Alternatively, ngcc's `ReflectionHost`s could be updated to do
this transformation (`decorators: []` --> `decorators: null`) when
reflecting on class members, but this would require changes in many
places and be less future-proof.

For example, given a class such as:

```ts
class Foo {
  @input() get bar() { return 'bar'; }
  set bar(value: any) {}
}
```

...previously the generated `setClassMetadata()` call would look like:

```ts
ɵsetClassMetadata(..., {
  bar: [{type: Input}],
  bar: [],
});
```

The same class will now result in a call like:

```ts
ɵsetClassMetadata(..., {
  bar: [{type: Input}],
});
```

Fixes #30569

PR Close #33514
kara added a commit that referenced this pull request Nov 13, 2019
Previously, ngcc's `Renderer` would add some constants in the processed
files which were emitted as ES2015 code (e.g. `const` declarations).
This would result in invalid ES5 generated code that would break when
run on browsers that do not support the emitted format.

This commit fixes it by adding a `printStatement()` method to
`RenderingFormatter`, which can convert statements to JavaScript code in
a suitable format for the corresponding `RenderingFormatter`.
Additionally, the `translateExpression()` and `translateStatement()`
ngtsc helper methods are augmented to accept an extra hint to know
whether the code needs to be translated to ES5 format or not.

Fixes #32665

PR Close #33514
@gkalpak gkalpak deleted the gkalpak:fix-ngcc-es5-code branch Nov 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.