feat(ivy): ngcc - recognize static properties on the outer symbol in ES5 by JoostK · Pull Request #30795 · angular/angular

JoostK · 2019-05-31T22:50:32Z

feat(ivy): ngcc - recognize static properties on the outer symbol in ES5

Packages that have been compiled using an older version of TypeScript
can have their decorators at the top-level of the ES5 bundles, instead
of inside the IIFE that is emitted for the class. Before this change,
ngcc only took static property assignments inside the IIFE into account,
therefore missing the decorators that were assigned at the top-level.

This commit extends the ES5 host to look for static properties in two
places. Testcases for all bundle formats that contain ES5 have been added
to ensure that this works in the various flavours.

A patch is included to support UMD bundles. The UMD factory affects how
TypeScripts binds the static properties to symbols, see the docblock of
the patch function for more details.

googlebot · 2019-05-31T22:50:41Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

kara · 2019-06-03T15:02:15Z

@JoostK Can you comment on what is blocking this PR specifically?

JoostK · 2019-06-03T16:01:22Z

Blocked on #30591.

petebacondarwin · 2019-06-06T11:00:11Z

@JoostK - does this PR need to sit on top of mine? Perhaps you could try it on top of master and then we can land it sooner...?

JoostK · 2019-06-06T12:52:41Z

@petebacondarwin I did it precautionary to avoid conflicts with your massive PR. If you don't think it would be too painful to rebase your PR on top of this one I can surely apply the changes on top of master.

Also, are we fine with the TypeScript patch that is in here? Any concerns regarding the patch may hold off merging this.

petebacondarwin · 2019-06-06T15:09:40Z

I'm happy for my PR to be rebased on top of this.
Regarding the patch... ideally we should try to sniff for the incorrect behaviour before patching. This way we will not unnecessarily patch TS once it is fixed at source.

Packages that have been compiled using an older version of TypeScript can have their decorators at the top-level of the ES5 bundles, instead of inside the IIFE that is emitted for the class. Before this change, ngcc only took static property assignments inside the IIFE into account, therefore missing the decorators that were assigned at the top-level. This commit extends the ES5 host to look for static properties in two places. Testcases for all bundle formats that contain ES5 have been added to ensure that this works in the various flavours. A patch is included to support UMD bundles. The UMD factory affects how TypeScripts binds the static properties to symbols, see the docblock of the patch function for more details.

…bol in ES5

googlebot · 2019-06-07T20:17:29Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

petebacondarwin · 2019-06-07T20:49:50Z

+   * Try to retrieve the symbol of a static property on a class.
+   *
+   * In ES5, a static property can either be set on the inner function declaration inside the class'
+   * IIFE, or it can be set on the outer variable declaration. Therefore, the ES2015 host's behavior


Do you mean Esm5ReflectionHost's behaviour?

I don't think so, although the current phrasing is easily misinterpreted I guess. Since the ES2015 host is still always used for actually reading the static prop from a symbol (by calling super), I meant to say that the ES5 host extends on that behaviour by looking in two places.

petebacondarwin · 2019-06-07T21:13:21Z

+})));`,
+};
+
+const CTOR_DECORATORS_ARRAY_FILE = {


This doesn't appear to be used??

Correct, got in when cherry-picking from when this commit was on top of yours. Will remove.

petebacondarwin · 2019-06-07T21:13:56Z

+  `,
+};
+
+const CTOR_DECORATORS_ARRAY_FILE = {


petebacondarwin

A comment to fix and some unwanted code but otherwise LGTM.

alxhub

pete's comments, then this looks good!

alxhub · 2019-06-07T21:33:52Z

+  }
+
+  const originalGetExpandoInitializer = (ts as any).getExpandoInitializer;
+  (ts as any).getExpandoInitializer =


Add a comment explaining what this function is.

Also, let's have a graceful failure mode if the function doesn't exist. I think throwing an error explaining that the TS version is incompatible with ngcc is a good approach. It's not likely but TS is free to remove this API whenever they please, and ngcc shouldn't silently stop working or crash without a clear message if they do.

Do you feel the docblock is not sufficient here?

Regarding the verification, that's a good idea. I will take it one step further and actually use the verification function above to assert that applying the patch has the desired effect.

The docblock is fine 👍 that sounds good to me!

…bol in ES5

angular-automatic-lock-bot · 2019-09-15T04:53:34Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

JoostK added type: bug/fix feature Label used to distinguish feature request from other issues state: blocked effort1: hours workaround4: none freq1: low severity3: broken target: major This PR is targeted for the next major release risk: medium labels May 31, 2019

JoostK requested review from a team May 31, 2019 22:50

ngbot Bot added this to the Backlog milestone May 31, 2019

googlebot added the cla: no label May 31, 2019

JoostK mentioned this pull request May 31, 2019

Compilation error in Ivy mode with 8.0.0-rc.5 #30680

Closed

JoostK force-pushed the ngcc-top-level-decorators branch from d997fc8 to 566b109 Compare May 31, 2019 23:04

This was referenced Jun 3, 2019

Enable ivy in angular 8 - production build got failed #30818

Closed

expando fields not added to symbol exports microsoft/TypeScript#31778

Open

JoostK added 3 commits June 7, 2019 21:09

fixup! feat(ivy): ngcc - recognize static properties on the outer sym…

a374a63

…bol in ES5

fixup! feat(ivy): ngcc - recognize static properties on the outer sym…

ca7565d

…bol in ES5

JoostK force-pushed the ngcc-top-level-decorators branch from 566b109 to ca7565d Compare June 7, 2019 20:17

googlebot added cla: yes and removed cla: no labels Jun 7, 2019

JoostK added action: review The PR is still awaiting reviews from at least one requested reviewer and removed state: blocked labels Jun 7, 2019

JoostK requested a review from petebacondarwin June 7, 2019 20:21

petebacondarwin reviewed Jun 7, 2019

View reviewed changes

petebacondarwin approved these changes Jun 7, 2019

View reviewed changes

petebacondarwin added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Jun 7, 2019

alxhub requested changes Jun 7, 2019

View reviewed changes

JoostK force-pushed the ngcc-top-level-decorators branch from a47e561 to de8deec Compare June 10, 2019 10:52

JoostK added action: review The PR is still awaiting reviews from at least one requested reviewer and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Jun 10, 2019

fixup! feat(ivy): ngcc - recognize static properties on the outer sym…

2a369e9

…bol in ES5

JoostK force-pushed the ngcc-top-level-decorators branch from de8deec to 2a369e9 Compare June 10, 2019 11:01

alxhub approved these changes Jun 14, 2019

View reviewed changes

JoostK added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Jun 14, 2019

AndrewKushnir closed this in 6fbfb5a Jun 14, 2019

angular-automatic-lock-bot Bot locked and limited conversation to collaborators Sep 15, 2019

JoostK deleted the ngcc-top-level-decorators branch September 15, 2019 13:05

Conversation

JoostK commented May 31, 2019

Uh oh!

googlebot commented May 31, 2019

Uh oh!

kara commented Jun 3, 2019

Uh oh!

JoostK commented Jun 3, 2019

Uh oh!

petebacondarwin commented Jun 6, 2019

Uh oh!

JoostK commented Jun 6, 2019

Uh oh!

petebacondarwin commented Jun 6, 2019

Uh oh!

googlebot commented Jun 7, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petebacondarwin left a comment

Choose a reason for hiding this comment

Uh oh!

alxhub left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

angular-automatic-lock-bot Bot commented Sep 15, 2019

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants