ngcc: implement ivy switch rendering by petebacondarwin · Pull Request #25534 · angular/angular

petebacondarwin · 2018-08-17T06:56:41Z

This is related to #25238

petebacondarwin · 2018-08-17T15:45:02Z

One concern I have is whether there will be additional places that need updating outside of the AnalyzedFiles that get passed to the Renderer... Any thoughts @alxhub / @mhevery ?

mhevery

There seems to be other unrelated changes in this PR, but the __PRE_NGCC__ rewrite to __POST_NGCC__ looks good to me.

mhevery · 2018-08-17T16:22:53Z

As of right now this switch is in @angular/core only. Should we short circuit this for perf reasons? Is there an argument to be made to enable such switch for others?

I need to test this but I suspect that the bottleneck for performance is creating the TS compilation, which is necessary for the rest of the work. So I believe that parsing for these markers will make little performance difference.

I suggest that we have a period of performance tuning once the functionality is in place. I think that there is potentially a lot of duplication of work, especially in the ngtsc resolver and the reflection hosts right now that would benefit from some caching strategies.

petebacondarwin · 2018-08-17T18:16:33Z

The other changes were to simplify the rendering tests, without which the changes to the tests would have got yet more complicated.

gkalpak

Some minor comments. LGTM otherwise 👍

gkalpak · 2018-08-20T12:46:03Z

Why not make this a method on the ReflectionHost. Having both standalone functions and Class methods gets confusing (imo).

We have lots of type constraint functions in the ngcc and ngtsc code. They are all standalone.

gkalpak · 2018-08-20T12:47:58Z

This sounds more like a visitor than a worker 😁

gkalpak · 2018-08-20T12:48:26Z

a all --> all

gkalpak · 2018-08-20T12:50:42Z

+should (?)

gkalpak · 2018-08-20T12:56:56Z

I wouldn't mind a test with __PRE_NGCC__ at the beginning of the identifier 😁

gkalpak · 2018-08-20T12:58:07Z

I wouldn't mind a test with __PRE_NGCC__ at the beginning of the identifier 😁

gkalpak · 2018-08-20T13:13:30Z

Using a helper function to strip indentation makes the code look (and get folded) much better. I usually have:

const stripIndentation = (input: string): string => { const lines = input.replace(/^ *\n/, '').replace(/\n *$/, '').split('\n'); const minIndentation = Math.min(...lines. filter(l => !/^ *$/.test(l)). map(l => /^ */.exec(l)![0].length)); const re = new RegExp(`^ {0,${minIndentation}}`); return lines.map(l => l.replace(re, '')).join('\n'); };

Fair enough. This has been a worry for me for a while. I'll give this a go.

why the two replaces? Blank lines get stripped in the filter right?

I think we should add that helper in a separate PR (since it would hit a lot of the tests).

SGTM.

why the two replaces? Blank lines get stripped in the filter right?

The two replace remove leading/trailing empty/whitespace-only lines from the output. The filter is only used to ignore whitespace-only lines for computing the min indentation.

gkalpak · 2018-08-20T13:14:53Z

+should (?)

petebacondarwin · 2018-08-21T06:24:20Z

Can a Googler run the presubmit please?

petebacondarwin · 2018-08-23T08:16:34Z

Rebased on master. Hoping @alxhub can get this through G3 etc.

mary-poppins · 2018-08-23T08:22:13Z

You can preview 059a9e1 at https://pr25534-059a9e1.ngbuilds.io/.

mary-poppins · 2018-08-23T08:49:31Z

You can preview ae1b36f at https://pr25534-ae1b36f.ngbuilds.io/.

alxhub · 2018-08-27T16:54:26Z

I'm becoming less of a fan of this approach of testing. It's really hard to look at this test and figure out whether it's correct.

Can we start thinking of a way to make this more straightforward? It doesn't have to happen in this PR.

alxhub · 2018-08-27T16:55:31Z

We should implement a performance optimization where we check module.getText() for PRE_NGCC_MARER via regex or .indexOf. Only if the test is positive should we walk the AST to actually find them.

This method will be used to find all the places where the "ivy switch" will occur. See angular#25238

This supports the "ngcc ivy switch" specified in angular#25238.

Also incorporates a refactoring of the tests to make them less fragile.

Only parse the AST for ngcc ivy switch constants if the marker is not found in the module text.

mary-poppins · 2018-08-28T08:33:58Z

You can preview 29457cc at https://pr25534-29457cc.ngbuilds.io/.

petebacondarwin · 2018-08-28T09:23:35Z

@alxhub can you help get the G3 presubmit green so that we can merge this?

mhevery · 2018-08-31T06:55:21Z

http://test/OCL:211042347:BASE:211042348:1535698506772:8183a8d4

…25534) PR Close #25534

) PR Close #25534

This supports the "ngcc ivy switch" specified in #25238. PR Close #25534

Also incorporates a refactoring of the tests to make them less fragile. PR Close #25534

Only parse the AST for ngcc ivy switch constants if the marker is not found in the module text. PR Close #25534

angular-automatic-lock-bot · 2019-09-14T00:22:32Z

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.}

petebacondarwin added feature Label used to distinguish feature request from other issues action: review The PR is still awaiting reviews from at least one requested reviewer comp: ivy labels Aug 17, 2018

petebacondarwin added this to the Ivy milestone Aug 17, 2018

petebacondarwin requested review from alxhub and mhevery August 17, 2018 06:56

googlebot added the cla: yes label Aug 17, 2018

petebacondarwin force-pushed the ngcc-ivy-switch branch from b858548 to 5038aa1 Compare August 17, 2018 07:28

petebacondarwin added the target: major This PR is targeted for the next major release label Aug 17, 2018

mhevery approved these changes Aug 17, 2018

View reviewed changes

gkalpak approved these changes Aug 20, 2018

View reviewed changes

petebacondarwin added merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Aug 21, 2018

petebacondarwin force-pushed the ngcc-ivy-switch branch from 6d7e15a to 059a9e1 Compare August 23, 2018 08:15

petebacondarwin force-pushed the ngcc-ivy-switch branch from 059a9e1 to ae1b36f Compare August 23, 2018 08:42

petebacondarwin assigned alxhub Aug 23, 2018

alxhub approved these changes Aug 27, 2018

View reviewed changes

petebacondarwin added 6 commits August 28, 2018 08:45

feat(ivy): implement NgccReflectionHost.getSwitchableDeclarations()

a5173e8

This method will be used to find all the places where the "ivy switch" will occur. See angular#25238

test(ivy): refactor Esm2015Renderer tests to make them less fragile

3b7df99

test(ivy): refactor Esm5Renderer tests to make them less fragile

4a51bb1

feat(ivy): implement Renderer.getSwitchableDeclarations

9a0c285

This supports the "ngcc ivy switch" specified in angular#25238.

test(ivy): test Esm5Renderer.getSwitchableDeclarations

a5f679f

Also incorporates a refactoring of the tests to make them less fragile.

perf(ivy): speed up ngcc ivy switch processing

29457cc

Only parse the AST for ngcc ivy switch constants if the marker is not found in the module text.

petebacondarwin force-pushed the ngcc-ivy-switch branch from ae1b36f to 29457cc Compare August 28, 2018 08:26

petebacondarwin added action: merge The PR is ready for merge by the caretaker and removed merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels Aug 28, 2018

mhevery closed this in 6f168b7 Aug 31, 2018

mhevery pushed a commit that referenced this pull request Aug 31, 2018

test(ivy): refactor Esm2015Renderer tests to make them less fragile (#…

7304748

…25534) PR Close #25534

mhevery pushed a commit that referenced this pull request Aug 31, 2018

test(ivy): refactor Esm5Renderer tests to make them less fragile (#25534

e73e864

) PR Close #25534

mhevery pushed a commit that referenced this pull request Aug 31, 2018

feat(ivy): implement Renderer.getSwitchableDeclarations (#25534)

26cd9f5

This supports the "ngcc ivy switch" specified in #25238. PR Close #25534

mhevery pushed a commit that referenced this pull request Aug 31, 2018

test(ivy): test Esm5Renderer.getSwitchableDeclarations (#25534)

e964319

Also incorporates a refactoring of the tests to make them less fragile. PR Close #25534

mhevery pushed a commit that referenced this pull request Aug 31, 2018

perf(ivy): speed up ngcc ivy switch processing (#25534)

20b9c61

Only parse the AST for ngcc ivy switch constants if the marker is not found in the module text. PR Close #25534

petebacondarwin deleted the ngcc-ivy-switch branch August 31, 2018 18:29

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

Conversation

petebacondarwin commented Aug 17, 2018

Uh oh!

petebacondarwin commented Aug 17, 2018

Uh oh!

mhevery 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!

petebacondarwin commented Aug 17, 2018

Uh oh!

gkalpak 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!

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!

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!

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 commented Aug 21, 2018

Uh oh!

petebacondarwin commented Aug 23, 2018

Uh oh!

mary-poppins commented Aug 23, 2018

Uh oh!

mary-poppins commented Aug 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alxhub Aug 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mary-poppins commented Aug 28, 2018

Uh oh!

petebacondarwin commented Aug 28, 2018

Uh oh!

mhevery commented Aug 31, 2018

Uh oh!

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

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

alxhub Aug 27, 2018 •

edited

Loading