feat(core): add automatic migration from Renderer to Renderer2 #30936

crisbeto · 2019-06-09T12:53:14Z

Adds a schematic and tslint rule that automatically migrate the consumer from Renderer to Renderer2. Supports:

Renaming imports.
Renaming property and method argument types.
Casting to Renderer.
Mapping all of the methods from the Renderer to Renderer2.

Note that some of the Renderer methods don't map cleanly between renderers. In these cases the migration adds a helper function at the bottom of the file which ensures that we generate valid code with the same return value as before. E.g. here's what the migration for createText looks like.

Before:

class SomeComponent {
  createAndAddText() {
    const node = this._renderer.createText(this._element.nativeElement, 'hello');
    node.textContent += ' world';
  }
}

After:

class SomeComponent {
  createAndAddText() {
    const node = __rendererCreateTextHelper(this._renderer, this._element.nativeElement, 'hello');
    node.textContent += ' world';
  }
}

function __rendererCreateTextHelper(renderer: any, parent: any, value: any) {
  const node = renderer.createText(value);
  if (parent) {
    renderer.appendChild(parent, node);
  }
  return node;
}

This PR resolves FW-1344.

kara

Thanks for doing all this work! Seems like it is pretty close! Some thoughts below

packages/core/schematics/migrations/renderer-to-renderer2/README.md

packages/core/schematics/migrations/renderer-to-renderer2/google3/rendererToRenderer2Rule.ts

packages/core/schematics/migrations/renderer-to-renderer2/helpers.ts

packages/core/schematics/test/renderer_to_renderer2_migration_spec.ts

packages/core/schematics/migrations/renderer-to-renderer2/index.ts

packages/core/schematics/test/renderer_to_renderer2_migration_spec.ts

packages/core/schematics/migrations/renderer-to-renderer2/helpers.ts

packages/core/schematics/migrations/renderer-to-renderer2/google3/rendererToRenderer2Rule.ts

packages/core/schematics/migrations/renderer-to-renderer2/index.ts

packages/core/schematics/migrations/renderer-to-renderer2/helpers.ts

packages/core/schematics/migrations/renderer-to-renderer2/index.ts

packages/core/schematics/migrations/renderer-to-renderer2/util.ts

packages/core/schematics/migrations/renderer-to-renderer2/index.ts

packages/core/schematics/migrations/renderer-to-renderer2/migration.ts

crisbeto · 2019-06-19T17:24:28Z

@kara @jelbourn @devversion I've addressed all of the feedback. Can you take another look?

alexeagle · 2019-06-19T18:32:12Z

I'll tsunami this over google3 code now to look for broken corner cases

alexeagle · 2019-06-19T21:30:42Z

This only affected 36 files at Google, so it feels like we don't need to polish the migration that much in order to land it in g3. A bunch of these could just be fixed manually in the couple places they occur, but I'll list them anyway:

in google3 we have a global type AnyDuringAngularIvyMigration - we shouldn't create types inline like

(also this added type isn't referenced in the file)
Here's an error that the forwardRef return type isn't updated:
this is a nit: we added conditional logic here for size even though the typechecker could have told us that it's non-null in this location

and here

and here
A comment got duplicated:

alexeagle · 2019-06-19T22:10:56Z

one more: a reference to ngRendererCreateElementHelper was added but the symbol isn't imported

alexeagle · 2019-06-19T22:53:06Z

I'll do a global presubmit run across google3 where I hide the Renderer symbol to verify the schematic caught all usages.

One thing we need to do in order to land this is update packages/platform-browser-dynamic/src/compiler_reflector.ts which currently has a reference to Renderer - can we remove that in this PR as well? Maybe it has broader implications and should be done separately; I'm not sure

kara · 2019-06-20T00:05:26Z

@alexeagle Let's land the platform-browser change in a follow-up?

packages/core/schematics/migrations/renderer-to-renderer2/index.ts

packages/core/schematics/migrations/renderer-to-renderer2/helpers.ts

alexeagle · 2019-06-20T14:26:53Z

#31162 has the removal of Renderer from platform-browser-dynamic

alexeagle · 2019-06-20T20:26:43Z

I ran a global presubmit across all of Google code and confirmed it's green: this migration found all the locations where Renderer was used.

crisbeto · 2019-06-29T12:30:42Z

@kara can you take another look? I've updated it to incorporate yours and Paul's feedback. I've also added handling for forwardRef based on the result from the G3 presubmit, but I wasn't able to reproduce the case that Alex saw where the helpers weren't inserted in one file. Finally, I added some extra logic to the setElementStyle migration so it uses the type checker to try and infer the value so we generate a ternary only if the value is ambiguous.

packages/core/schematics/migrations/renderer-to-renderer2/helpers.ts

Adds a schematic and tslint rule that automatically migrate the consumer from `Renderer` to `Renderer2`. Supports: * Renaming imports. * Renaming property and method argument types. * Casting to `Renderer`. * Mapping all of the methods from the `Renderer` to `Renderer2`. Note that some of the `Renderer` methods don't map cleanly between renderers. In these cases the migration adds a helper function at the bottom of the file which ensures that we generate valid code with the same return value as before. E.g. here's what the migration for `createText` looks like. Before: ``` class SomeComponent { createAndAddText() { const node = this._renderer.createText(this._element.nativeElement, 'hello'); node.textContent += ' world'; } } ``` After: ``` class SomeComponent { createAndAddText() { const node = __rendererCreateTextHelper(this._renderer, this._element.nativeElement, 'hello'); node.textContent += ' world'; } } function __rendererCreateTextHelper(renderer: any, parent: any, value: any) { const node = renderer.createText(value); if (parent) { renderer.appendChild(parent, node); } return node; } ``` This PR resolves FW-1344.

crisbeto · 2019-07-02T19:15:40Z

Addressed the latest set of comments.

kara

LGTM

kara · 2019-07-02T19:30:52Z

presubmit

kara · 2019-07-02T21:28:02Z

Merge-assistance: codefresh OOM

angular-automatic-lock-bot · 2019-09-15T06:51:58Z

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

googlebot added the cla: yes label Jun 9, 2019

crisbeto force-pushed the FW-1344/renderer-migration branch 2 times, most recently from 491965d to 2c570bd Compare June 9, 2019 13:38

crisbeto added action: review The PR is still awaiting reviews from at least one requested reviewer area: core Issues related to the framework runtime target: major This PR is targeted for the next major release labels Jun 9, 2019

ngbot bot added this to the needsTriage milestone Jun 9, 2019

crisbeto marked this pull request as ready for review June 9, 2019 13:55

crisbeto requested a review from a team as a code owner June 9, 2019 13:55

kara requested changes Jun 13, 2019

View reviewed changes

kara reviewed Jun 13, 2019

View reviewed changes

packages/core/schematics/test/renderer_to_renderer2_migration_spec.ts Show resolved Hide resolved

jelbourn reviewed Jun 13, 2019

View reviewed changes

kara requested a review from devversion June 14, 2019 20:36

devversion reviewed Jun 17, 2019

View reviewed changes

kara 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 18, 2019

kara assigned crisbeto Jun 18, 2019

crisbeto force-pushed the FW-1344/renderer-migration branch from 2c570bd to 8cbfd01 Compare June 19, 2019 17:23

crisbeto 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 19, 2019

kara reviewed Jun 20, 2019

View reviewed changes

kara added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jun 20, 2019

crisbeto force-pushed the FW-1344/renderer-migration branch 2 times, most recently from d123ed4 to 70289a8 Compare June 29, 2019 12:23

crisbeto removed their assignment Jun 29, 2019

crisbeto removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jun 29, 2019

crisbeto force-pushed the FW-1344/renderer-migration branch 2 times, most recently from 81d82a0 to bd5e5db Compare June 29, 2019 21:55

kara reviewed Jul 2, 2019

View reviewed changes

packages/core/schematics/migrations/renderer-to-renderer2/helpers.ts Outdated Show resolved Hide resolved

packages/core/schematics/migrations/renderer-to-renderer2/helpers.ts Outdated Show resolved Hide resolved

kara 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 Jul 2, 2019

kara assigned crisbeto Jul 2, 2019

crisbeto added 3 commits July 2, 2019 20:46

fixup! feat(core): add automatic migration from Renderer to Renderer2

ab1bbfd

fixup! feat(core): add automatic migration from Renderer to Renderer2

9ed1a44

crisbeto force-pushed the FW-1344/renderer-migration branch from bd5e5db to 9ed1a44 Compare July 2, 2019 18:53

crisbeto 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 Jul 2, 2019

crisbeto removed their assignment Jul 2, 2019

kara approved these changes Jul 2, 2019

View reviewed changes

kara added action: merge The PR is ready for merge by the caretaker action: presubmit The PR is in need of a google3 presubmit and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Jul 2, 2019

kara 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: presubmit The PR is in need of a google3 presubmit labels Jul 2, 2019

alxhub closed this in c095597 Jul 3, 2019

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(core): add automatic migration from Renderer to Renderer2 #30936

feat(core): add automatic migration from Renderer to Renderer2 #30936

crisbeto commented Jun 9, 2019

kara left a comment

crisbeto commented Jun 19, 2019

alexeagle commented Jun 19, 2019

alexeagle commented Jun 19, 2019

alexeagle commented Jun 19, 2019

alexeagle commented Jun 19, 2019

kara commented Jun 20, 2019

alexeagle commented Jun 20, 2019

alexeagle commented Jun 20, 2019

crisbeto commented Jun 29, 2019 •

edited

crisbeto commented Jul 2, 2019

kara left a comment

kara commented Jul 2, 2019

kara commented Jul 2, 2019 •

edited

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

feat(core): add automatic migration from Renderer to Renderer2 #30936

feat(core): add automatic migration from Renderer to Renderer2 #30936

Conversation

crisbeto commented Jun 9, 2019

kara left a comment

Choose a reason for hiding this comment

crisbeto commented Jun 19, 2019

alexeagle commented Jun 19, 2019

alexeagle commented Jun 19, 2019

alexeagle commented Jun 19, 2019

alexeagle commented Jun 19, 2019

kara commented Jun 20, 2019

alexeagle commented Jun 20, 2019

alexeagle commented Jun 20, 2019

crisbeto commented Jun 29, 2019 • edited

crisbeto commented Jul 2, 2019

kara left a comment

Choose a reason for hiding this comment

kara commented Jul 2, 2019

kara commented Jul 2, 2019 • edited

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

crisbeto commented Jun 29, 2019 •

edited

kara commented Jul 2, 2019 •

edited