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

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

Closed
wants to merge 3 commits into from

Conversation

@crisbeto
Copy link
Member

commented Jun 9, 2019

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.

@googlebot googlebot added the cla: yes label Jun 9, 2019

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

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

@crisbeto crisbeto marked this pull request as ready for review Jun 9, 2019

@crisbeto crisbeto requested a review from angular/fw-core as a code owner Jun 9, 2019

@kara kara requested a review from devversion Jun 14, 2019

@crisbeto crisbeto force-pushed the crisbeto:FW-1344/renderer-migration branch from 2c570bd to 8cbfd01 Jun 19, 2019

@crisbeto

This comment has been minimized.

Copy link
Member Author

commented Jun 19, 2019

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

@alexeagle

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2019

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

@alexeagle

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2019

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:

  1. in google3 we have a global type AnyDuringAngularIvyMigration - we shouldn't create types inline like
    image
    (also this added type isn't referenced in the file)

  2. Here's an error that the forwardRef return type isn't updated:
    image

  3. 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
    image
    and here
    image
    and here
    image
    image
    image

  4. A comment got duplicated:
    image

@alexeagle

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2019

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

image

@alexeagle

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2019

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

@alexeagle

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2019

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

@alexeagle

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2019

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 crisbeto force-pushed the crisbeto:FW-1344/renderer-migration branch 2 times, most recently from d123ed4 to 70289a8 Jun 26, 2019

@crisbeto

This comment has been minimized.

Copy link
Member Author

commented Jun 29, 2019

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

@crisbeto crisbeto removed their assignment Jun 29, 2019

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

crisbeto added some commits Jun 9, 2019

feat(core): add automatic migration from Renderer to Renderer2
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 crisbeto force-pushed the crisbeto:FW-1344/renderer-migration branch from bd5e5db to 9ed1a44 Jul 2, 2019

@crisbeto crisbeto removed their assignment Jul 2, 2019

@crisbeto

This comment has been minimized.

Copy link
Member Author

commented Jul 2, 2019

Addressed the latest set of comments.

@kara

kara approved these changes Jul 2, 2019

Copy link
Contributor

left a comment

LGTM

@kara

This comment has been minimized.

Copy link
Contributor

commented Jul 2, 2019

@kara

This comment has been minimized.

Copy link
Contributor

commented Jul 2, 2019

Merge-assistance: codefresh OOM

@alxhub alxhub closed this in c095597 Jul 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.