ngcc: implement ivy switch rendering #25534

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

packages/compiler-cli/src/ngcc/src/host/esm2015_host.ts

+   * @returns An array of variable declarations that match.
+   */
+  getSwitchableDeclarations(module: ts.Node): SwitchableVariableDeclaration[] {
+    return findAll(module, isSwitchableVariableDeclaration);


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

packages/compiler-cli/src/ngcc/src/host/ngcc_host.ts

+export const POST_NGCC_MARKER = '__POST_NGCC__';
+
+export type SwitchableVariableDeclaration = ts.VariableDeclaration & {initializer: ts.Identifier};
+export function isSwitchableVariableDeclaration(node: ts.Node):


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

packages/compiler-cli/src/ngcc/src/utils.ts

+ */
+export function findAll<T>(node: ts.Node, test: (node: ts.Node) => node is ts.Node & T): T[] {
+  const nodes: T[] = [];
+  findAllWorker(node);


This sounds more like a visitor than a worker 😁

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

packages/compiler-cli/src/ngcc/src/utils.ts

+}
+
+/**
+ * Parse down the AST and capture a all the nodes that satisfy the test.


a all --> all

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

packages/compiler-cli/src/ngcc/test/rendering/esm2015_renderer_spec.ts

@@ -82,6 +94,27 @@ export class A {}`);
    });
  });

+  describe('rewriteSwitchableDeclarations', () => {
+    it('switch marked declaration initializers', () => {


+should (?)

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

packages/compiler-cli/src/ngcc/test/rendering/esm2015_renderer_spec.ts

@@ -46,6 +46,18 @@ export class C {}
 C.decorators = [
  { type: Directive, args: [{ selector: '[c]' }] },
 ];
+let compileNgModuleFactory = compileNgModuleFactory__PRE_NGCC__;


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

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

packages/compiler-cli/src/ngcc/test/host/esm2015_host_spec.ts

+const MARKER_FILE = {
+  name: '/marker.js',
+  contents: `
+    let compileNgModuleFactory = compileNgModuleFactory__PRE_NGCC__;


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

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

packages/compiler-cli/src/ngcc/test/rendering/esm5_renderer_spec.ts

-        contents: `
+const PROGRAM = {
+  name: 'some/file.js',
+  contents: `


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

packages/compiler-cli/src/ngcc/test/rendering/esm5_renderer_spec.ts

    });
  });

+  describe('rewriteSwitchableDeclarations', () => {
+    it('switch marked declaration initializers', () => {


+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

packages/compiler-cli/src/ngcc/test/rendering/renderer_spec.ts

@@ -68,7 +71,7 @@ describe('Renderer', () => {
    ]
  });
  const RENDERED_CONTENTS =
-      `\n// REMOVE DECORATORS\n\n// ADD IMPORTS\n\n// ADD CONSTANTS\n\n// ADD DEFINITIONS\n` +
+      `\n// REWRITTEN DECLARATIONS\n\n// REMOVE DECORATORS\n\n// ADD IMPORTS\n\n// ADD CONSTANTS\n\n// ADD DEFINITIONS\n` +


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

packages/compiler-cli/src/ngcc/src/host/fesm2015_host.ts

+   * @returns An array of variable declarations that match.
+   */
+  getSwitchableDeclarations(module: ts.Node): SwitchableVariableDeclaration[] {
+    return findAll(module, isSwitchableVariableDeclaration);


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#25534) This method will be used to find all the places where the "ivy switch" will occur. See angular#25238 PR Close angular#25534

…ngular#25534) PR Close angular#25534

…ular#25534) PR Close angular#25534

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

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

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

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

…ngular#25534) PR Close angular#25534

…ular#25534) PR Close angular#25534

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

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

Only parse the AST for ngcc ivy switch constants if the marker is not found in the module text. PR Close angular#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 Issue that requests a new feature 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 mhevery and alxhub 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

petebacondarwin added a commit to petebacondarwin/angular that referenced this pull request Sep 3, 2018

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

02ce963

…ngular#25534) PR Close angular#25534

petebacondarwin added a commit to petebacondarwin/angular that referenced this pull request Sep 3, 2018

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

1c9db34

…ular#25534) PR Close angular#25534

petebacondarwin added a commit to petebacondarwin/angular that referenced this pull request Sep 3, 2018

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

b49ece2

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

petebacondarwin added a commit to petebacondarwin/angular that referenced this pull request Sep 3, 2018

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

1002b06

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

petebacondarwin added a commit to petebacondarwin/angular that referenced this pull request Sep 3, 2018

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

fcbf53a

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

FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this pull request Jan 3, 2019

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

0dce18b

…ngular#25534) PR Close angular#25534

FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this pull request Jan 3, 2019

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

4ce2d6b

…ular#25534) PR Close angular#25534

FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this pull request Jan 3, 2019

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

516caed

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

FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this pull request Jan 3, 2019

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

a29c39a

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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ngcc: implement ivy switch rendering #25534

ngcc: implement ivy switch rendering #25534

petebacondarwin commented Aug 17, 2018

petebacondarwin commented Aug 17, 2018

mhevery left a comment

mhevery Aug 17, 2018

petebacondarwin Aug 17, 2018

petebacondarwin Aug 17, 2018

petebacondarwin commented Aug 17, 2018

gkalpak left a comment

gkalpak Aug 20, 2018

petebacondarwin Aug 20, 2018

gkalpak Aug 20, 2018

gkalpak Aug 20, 2018

gkalpak Aug 20, 2018

gkalpak Aug 20, 2018

gkalpak Aug 20, 2018

petebacondarwin Aug 20, 2018

petebacondarwin Aug 21, 2018

gkalpak Aug 20, 2018

petebacondarwin Aug 20, 2018

petebacondarwin Aug 21, 2018

petebacondarwin Aug 21, 2018

gkalpak Aug 21, 2018

petebacondarwin Aug 21, 2018

gkalpak Aug 20, 2018

petebacondarwin commented Aug 21, 2018

petebacondarwin commented Aug 23, 2018

mary-poppins commented Aug 23, 2018

mary-poppins commented Aug 23, 2018

alxhub Aug 27, 2018

alxhub Aug 27, 2018 •

edited

mary-poppins commented Aug 28, 2018

petebacondarwin commented Aug 28, 2018

mhevery commented Aug 31, 2018

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

ngcc: implement ivy switch rendering #25534

ngcc: implement ivy switch rendering #25534

Conversation

petebacondarwin commented Aug 17, 2018

petebacondarwin commented Aug 17, 2018

mhevery left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petebacondarwin commented Aug 17, 2018

gkalpak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petebacondarwin commented Aug 21, 2018

petebacondarwin commented Aug 23, 2018

mary-poppins commented Aug 23, 2018

mary-poppins commented Aug 23, 2018

Choose a reason for hiding this comment

alxhub Aug 27, 2018 • edited

Choose a reason for hiding this comment

mary-poppins commented Aug 28, 2018

petebacondarwin commented Aug 28, 2018

mhevery commented Aug 31, 2018

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

alxhub Aug 27, 2018 •

edited