feat(ivy): support injecting Renderer2 #25523

marclaval · 2018-08-16T16:02:38Z

This PR adds support for injecting Renderer2.

The Renderer2Adapter class is almost a copy/paste of the DefaultDomRenderer2 class that we have in the platorm-browser package, but without dependencies to EventManager and NgZone.
It feels wrong, but at the same time a ObjectOrientedRenderer3 is not linked to the DOM, in theory. The adapter should only know about Rnodes.

About the changes in the compiler, this is the first time I do some. So please review them carefully.

JoostK · 2018-08-16T16:31:35Z

packages/compiler/src/render3/r3_factory.ts

@@ -318,6 +326,8 @@ export function dependenciesFromGlobalMetadata(
        resolved = R3ResolvedDependencyType.Injector;
      } else if (dependency.isAttribute) {
        resolved = R3ResolvedDependencyType.Attribute;
+      } else if (tokenRef === renderer2) {


Could this be immediately following the other tokenRef guards above, its location below dependency.isAttribute seems a bit out of line to me.

JoostK · 2018-08-16T16:39:38Z

packages/core/src/render3/interfaces/renderer.ts

+                                                selectorOrNode;
+  }
+
+  parentNode(node: RNode): RNode { return (node as any).parentNode; }


This return type can't be correct, as the root node doesn't have a parent. Should include undefined or null?

JoostK · 2018-08-16T16:43:17Z

packages/core/src/render3/interfaces/renderer.ts

+      if (target === 'window') {
+        targetElement = window;
+      } else if (target === 'document') {
+        targetElement = this.renderer as Document;


Wasn't it possible for the renderer to be different across views? That would mean this isn't always the case.

This part of the API (event target as a string) is way too DOM specific. In fact, I think it shouldn't be supported in such an adapter.

mhevery · 2018-08-16T18:26:45Z

packages/core/src/render3/di.ts

+  if (!di.renderer2) {
+    const renderer = di.node.view[RENDERER];
+    di.renderer2 =
+        isProceduralRenderer(renderer) ? renderer as Renderer2 : new Renderer2Adapter(renderer);


Not sure I like that we have made Renderer2Adapter non tree-shakable. I think we should just return null. If your app wants Render2 we should bootstrap with Render2 rather than always pool in Renderer2Adapter which wont be used in vast majority of cases.

That's the drawback yes.

If we don't have this adapter, that would be a breaking change. Apps might crash at runtime if they move from Renderer2 to Renderer3, because themselves or one of their dependencies injects Renderer2.

mhevery · 2018-08-16T18:27:52Z

packages/core/src/render3/interfaces/injector.ts

@@ -83,6 +84,9 @@ export interface LInjector {
   * same instance.
   */
  changeDetectorRef: ChangeDetectorRef|null;
+
+  /** Stores the Renderer2 so subsequent injections of the Renderer2 get the same instance. */
+  renderer2: Renderer2|null;


I don't think we need to store this since it is same as view[RENDERER]

mhevery · 2018-08-16T18:29:15Z

packages/core/src/render3/interfaces/renderer.ts

+  'xmlns': 'http://www.w3.org/2000/xmlns/',
+};
+
+export class Renderer2Adapter extends Renderer2 {


I don't think we need this. I would just revert this.

Let's discuss this

mary-poppins · 2018-08-17T08:44:52Z

You can preview f37d5c7 at https://pr25523-f37d5c7.ngbuilds.io/.

mary-poppins · 2018-08-17T16:05:23Z

You can preview 3fcc4e2 at https://pr25523-3fcc4e2.ngbuilds.io/.

mary-poppins · 2018-08-20T08:11:26Z

You can preview c4a85e8 at https://pr25523-c4a85e8.ngbuilds.io/.

mary-poppins · 2018-08-20T08:45:16Z

You can preview 0f8c425 at https://pr25523-0f8c425.ngbuilds.io/.

mary-poppins · 2018-08-20T09:41:49Z

You can preview 00b6c41 at https://pr25523-00b6c41.ngbuilds.io/.

mary-poppins · 2018-08-21T07:39:13Z

You can preview 2ae0bb8 at https://pr25523-2ae0bb8.ngbuilds.io/.

mary-poppins · 2018-08-21T16:22:18Z

You can preview 9250e99 at https://pr25523-9250e99.ngbuilds.io/.

mary-poppins · 2018-08-22T08:06:15Z

You can preview c887a36 at https://pr25523-c887a36.ngbuilds.io/.

marclaval · 2018-08-23T09:07:00Z

@mhevery PR rebased and updated to your comments, PTAL

mary-poppins · 2018-08-24T15:25:55Z

You can preview 9550aec at https://pr25523-9550aec.ngbuilds.io/.

mary-poppins · 2018-08-24T15:59:48Z

You can preview 1eeb701 at https://pr25523-1eeb701.ngbuilds.io/.

mary-poppins · 2018-08-27T07:58:37Z

You can preview 6995caf at https://pr25523-6995caf.ngbuilds.io/.

mary-poppins · 2018-08-27T09:00:46Z

You can preview 286772b at https://pr25523-286772b.ngbuilds.io/.

mary-poppins · 2018-08-27T16:27:50Z

You can preview d81f211 at https://pr25523-d81f211.ngbuilds.io/.

mary-poppins · 2018-08-27T16:34:57Z

You can preview f4a43ba at https://pr25523-f4a43ba.ngbuilds.io/.

mary-poppins · 2018-08-28T07:48:18Z

You can preview ee6a132 at https://pr25523-ee6a132.ngbuilds.io/.

mhevery · 2018-08-28T17:35:02Z

http://test/OCL:210564490:BASE:210564566:1535477659881:35f16132

caretaker please check above and merge

mary-poppins · 2018-08-29T07:30:50Z

You can preview 4dea0d2 at https://pr25523-4dea0d2.ngbuilds.io/.

mary-poppins · 2018-08-29T14:02:14Z

You can preview 5885b2d at https://pr25523-5885b2d.ngbuilds.io/.

PR Close angular#25523

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

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

marclaval added action: review The PR is still awaiting reviews from at least one requested reviewer target: major This PR is targeted for the next major release comp: ivy labels Aug 16, 2018

marclaval requested review from mhevery and alxhub August 16, 2018 16:02

googlebot added the cla: yes label Aug 16, 2018

JoostK reviewed Aug 16, 2018

View reviewed changes

mhevery suggested changes Aug 16, 2018

View reviewed changes

marclaval force-pushed the injectRenderer2 branch from 477b6a2 to f37d5c7 Compare August 17, 2018 08:37

marclaval force-pushed the injectRenderer2 branch from f37d5c7 to 3fcc4e2 Compare August 17, 2018 15:56

marclaval force-pushed the injectRenderer2 branch from 3fcc4e2 to c4a85e8 Compare August 20, 2018 08:03

marclaval force-pushed the injectRenderer2 branch from c4a85e8 to 0f8c425 Compare August 20, 2018 08:34

marclaval force-pushed the injectRenderer2 branch from 0f8c425 to 00b6c41 Compare August 20, 2018 09:33

marclaval force-pushed the injectRenderer2 branch from 00b6c41 to 2ae0bb8 Compare August 21, 2018 07:30

marclaval force-pushed the injectRenderer2 branch from 2ae0bb8 to 9250e99 Compare August 21, 2018 16:15

marclaval force-pushed the injectRenderer2 branch from 9250e99 to c887a36 Compare August 22, 2018 07:57

marclaval force-pushed the injectRenderer2 branch from c887a36 to 9550aec Compare August 24, 2018 15:17

marclaval force-pushed the injectRenderer2 branch from 9550aec to 1eeb701 Compare August 24, 2018 15:51

marclaval force-pushed the injectRenderer2 branch from 1eeb701 to 6995caf Compare August 27, 2018 07:49

marclaval force-pushed the injectRenderer2 branch from 6995caf to 286772b Compare August 27, 2018 08:52

marclaval force-pushed the injectRenderer2 branch 2 times, most recently from d81f211 to f4a43ba Compare August 27, 2018 16:22

marclaval force-pushed the injectRenderer2 branch from f4a43ba to ee6a132 Compare August 28, 2018 07:38

mhevery approved these changes Aug 28, 2018

View reviewed changes

mhevery 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 Aug 28, 2018

mhevery added the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Aug 28, 2018

marclaval force-pushed the injectRenderer2 branch from ee6a132 to 4dea0d2 Compare August 29, 2018 07:22

marclaval force-pushed the injectRenderer2 branch from 4dea0d2 to 5885b2d Compare August 29, 2018 13:54

feat(ivy): support injecting Renderer2

5885b2d

marclaval removed the request for review from alxhub August 31, 2018 15:54

mhevery closed this in 00f1311 Aug 31, 2018

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

feat(ivy): support injecting Renderer2 (angular#25523)

21b62bc

PR Close angular#25523

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

feat(ivy): support injecting Renderer2 (angular#25523)

71b4efc

PR Close angular#25523

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

feat(ivy): support injecting Renderer2 #25523

feat(ivy): support injecting Renderer2 #25523

marclaval commented Aug 16, 2018

JoostK Aug 16, 2018

marclaval Aug 17, 2018

JoostK Aug 16, 2018

marclaval Aug 17, 2018

JoostK Aug 16, 2018

marclaval Aug 17, 2018

mhevery Aug 16, 2018

marclaval Aug 17, 2018

mhevery Aug 16, 2018

marclaval Aug 17, 2018

mhevery Aug 16, 2018

marclaval Aug 17, 2018

mary-poppins commented Aug 17, 2018

mary-poppins commented Aug 17, 2018

mary-poppins commented Aug 20, 2018

mary-poppins commented Aug 20, 2018

mary-poppins commented Aug 20, 2018

mary-poppins commented Aug 21, 2018

mary-poppins commented Aug 21, 2018

mary-poppins commented Aug 22, 2018

marclaval commented Aug 23, 2018

mary-poppins commented Aug 24, 2018

mary-poppins commented Aug 24, 2018

mary-poppins commented Aug 27, 2018

mary-poppins commented Aug 27, 2018

mary-poppins commented Aug 27, 2018

mary-poppins commented Aug 27, 2018

mary-poppins commented Aug 28, 2018

mhevery commented Aug 28, 2018

mary-poppins commented Aug 29, 2018

mary-poppins commented Aug 29, 2018

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

feat(ivy): support injecting Renderer2 #25523

feat(ivy): support injecting Renderer2 #25523

Conversation

marclaval commented Aug 16, 2018

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

mary-poppins commented Aug 17, 2018

mary-poppins commented Aug 17, 2018

mary-poppins commented Aug 20, 2018

mary-poppins commented Aug 20, 2018

mary-poppins commented Aug 20, 2018

mary-poppins commented Aug 21, 2018

mary-poppins commented Aug 21, 2018

mary-poppins commented Aug 22, 2018

marclaval commented Aug 23, 2018

mary-poppins commented Aug 24, 2018

mary-poppins commented Aug 24, 2018

mary-poppins commented Aug 27, 2018

mary-poppins commented Aug 27, 2018

mary-poppins commented Aug 27, 2018

mary-poppins commented Aug 27, 2018

mary-poppins commented Aug 28, 2018

mhevery commented Aug 28, 2018

mary-poppins commented Aug 29, 2018

mary-poppins commented Aug 29, 2018

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