fix(api): use only Renderer2 instances #391

ThomasBurleson · 2017-08-24T18:53:20Z

NgClass and NgStyle use Renderer1 instances; which has been deprecated. Instead use Renderer2 and a RendererAdapter.

Thx @mhevery for his solution!

Refs #386. Fixes #398, #408.

ThomasBurleson · 2017-08-24T18:56:19Z

@mhevery - would you review to confirm ?

PS Just review the last commit: ce7d828

ThomasBurleson · 2017-08-28T16:35:04Z

@jelbourn - ready for your review also.

ThomasBurleson · 2017-08-29T02:27:26Z

@crisbeto - would you mind reviewing this one ?

crisbeto · 2017-08-29T16:32:13Z

src/lib/api/core/renderer-adapter.ts

+  // we only use setElementStyle() and setElementClass()
+  // ******************************************************************
+
+  selectRootElement(): any { }


If these methods aren't meant to be used, consider throwing an error or removing the implementation so they fall back to the inherited ones from the Renderer?

We cannot remove the implementation... We want to to appear as a Renderer1 instance but built from a Renderer2. Renderer is an abstract class with many abstract methods. This is we have to implement them EVEN if ignored.

But we can throw error as not-implemented!

Fixed.

crisbeto · 2017-08-29T16:33:57Z

src/lib/utils/testing/custom-matchers.ts

+ * Curried value to function to check styles that are inline or in a stylesheet for the
+ * specified DOM element.
+ */
+function buildCompareStyleFunction(inlineOnly = true) {


It seems like these changes are repeated between this PR, #394 and #396. Is this intentional?

This ^ is because PR 391 will be rebased after #390 gets merged.

crisbeto · 2017-08-29T16:35:19Z

src/lib/utils/testing/dom-tools.ts

@@ -73,6 +76,14 @@ function hasClass(element: any, className: string): boolean {
  return element.classList.contains(className);
 }

+function hasAttribute(element: any, attributeName: string): boolean {
+  return element.hasAttribute(attributeName);


It doesn't look like most of these DOM utilities are doing much, aside from proxying the native functions with the exact same arguments.

crisbeto

LGTM

jelbourn · 2017-08-30T01:58:30Z

@ThomasBurleson ping me when this one is rebased

mhevery · 2017-08-31T21:20:48Z

src/lib/api/core/renderer-adapter.ts

+ * This is required for older versions of NgStyle and NgClass: which require
+ * the v1 API (but should use the v2 instances)
+ */
+export class RendererAdapter extends Renderer {


You are still referring to Renderer. I think you should drop that.

@mhevery - To manually construct a NgClass I must pass it an instance of a class that extends Renderer. Or am I missing something ?

Do you mean change RendererAdapter to HybridAdapter ?

The whole point of the issue is that the Renderer symbol is being removed entirely.

@jelbourn -If we do this, then we break support for Angular 4.x

// Create an instance NgClass Directive instance only if `ngClass=""` has NOT been defined on // the same host element; since the responsive variations may be defined... if ( !this._ngClassInstance ) { let adapter = new RendererAdapter(_renderer); this._ngClassInstance = new NgClass(_iterableDiffers, _keyValueDiffers, _ngEl, adapter); }

since NgClass in core (v4.3) expects a Renderer subclass.

export declare abstract class Renderer { }

So I am not sure WHAT to do here because the darn Renderer extends an abstract base class instead of implementing an interface.

Instead of creating an internal ngClass instance, I could just copy the class implementation methods from ngClass (in core), but that seems to be a clumsy solution. Perhaps that is the only viable solution ?

don't implement Renderer

Make sure that the Shape of the classes matches what Renderer expects (this should be noop since you implemented it.)

When passing it into NgClass just cast it to any so that the compiler is happy.

@mhevery

NgClass and NgStyle use Renderer1 instances; which has been deprecated. Instead use Renderer2 and a RendererAdapter. Thx @mhevery for his solution! Refs #386.

ThomasBurleson · 2017-09-01T11:41:04Z

@mhevery - Imports of Renderer are now gone. Your last recommendation was ridiculous-easy:

export class RendererAdapter { }

// ...

if ( !this._ngClassInstance ) {
    let adapter = new RendererAdapter(_renderer);
    this._ngClassInstance = new NgClass(_iterableDiffers, _keyValueDiffers, _ngEl, <any> adapter);
}

I was 'locked' into classic OOP and did not even try this ^ myself.

mhevery · 2017-09-01T18:56:51Z

Perfect LGTM

0x-r4bbit · 2017-09-07T13:20:40Z

ping me when this one is rebased

@jelbourn Ping! :)

Would be cool if we could get this in and release as beta.10 as this is a blocker right now to upgrade from Angular 5.0.0-beta.4.

angular-automatic-lock-bot · 2019-09-05T22:04:14Z

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 Aug 24, 2017

ThomasBurleson mentioned this pull request Aug 24, 2017

fix: update to RendererV2 interface #386

Closed

ThomasBurleson added the pr: needs presubmit (2nd) label Aug 24, 2017

ThomasBurleson modified the milestone: v2.0.0-beta.10 Aug 28, 2017

ThomasBurleson requested review from mhevery and jelbourn August 28, 2017 16:33

ThomasBurleson requested a review from crisbeto August 29, 2017 02:27

This was referenced Aug 29, 2017

regression: render.removeClass is not a function [5.0.0-beta.5] angular/angular#18922

Closed

ClassDirective throws renderer error with Angular 5.0.0-beta.5 #398

Closed

crisbeto reviewed Aug 29, 2017

View reviewed changes

ThomasBurleson force-pushed the thomas/fix-renderer2 branch from ce7d828 to cc542d1 Compare August 29, 2017 19:51

crisbeto approved these changes Aug 29, 2017

View reviewed changes

crisbeto added pr: lgtm This PR has been approved by the reviewer pr: merge ready This PR is ready for the caretaker to presubmit and merge labels Aug 29, 2017

ThomasBurleson force-pushed the thomas/fix-renderer2 branch from cc542d1 to 392545e Compare August 31, 2017 17:12

mhevery approved these changes Aug 31, 2017

View reviewed changes

fix(api): use only Renderer2 instances

c175adb

NgClass and NgStyle use Renderer1 instances; which has been deprecated. Instead use Renderer2 and a RendererAdapter. Thx @mhevery for his solution! Refs #386.

ThomasBurleson force-pushed the thomas/fix-renderer2 branch from 392545e to c175adb Compare September 1, 2017 11:37

tinayuangao merged commit 816d85a into master Sep 7, 2017

ThomasBurleson deleted the thomas/fix-renderer2 branch September 13, 2017 22:12

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(api): use only Renderer2 instances #391

fix(api): use only Renderer2 instances #391

ThomasBurleson commented Aug 24, 2017 •

edited

Loading

ThomasBurleson commented Aug 24, 2017 •

edited

Loading

ThomasBurleson commented Aug 28, 2017

ThomasBurleson commented Aug 29, 2017

crisbeto Aug 29, 2017

ThomasBurleson Aug 29, 2017

crisbeto Aug 29, 2017

ThomasBurleson Aug 29, 2017

crisbeto Aug 29, 2017

crisbeto left a comment

jelbourn commented Aug 30, 2017

mhevery Aug 31, 2017

ThomasBurleson Aug 31, 2017

ThomasBurleson Aug 31, 2017

jelbourn Aug 31, 2017

ThomasBurleson Aug 31, 2017 •

edited

Loading

ThomasBurleson Aug 31, 2017 •

edited

Loading

ThomasBurleson Aug 31, 2017 •

edited

Loading

mhevery Sep 1, 2017

ThomasBurleson commented Sep 1, 2017

mhevery commented Sep 1, 2017

0x-r4bbit commented Sep 7, 2017

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

fix(api): use only Renderer2 instances #391

fix(api): use only Renderer2 instances #391

Conversation

ThomasBurleson commented Aug 24, 2017 • edited Loading

ThomasBurleson commented Aug 24, 2017 • edited Loading

ThomasBurleson commented Aug 28, 2017

ThomasBurleson commented Aug 29, 2017

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

crisbeto left a comment

Choose a reason for hiding this comment

jelbourn commented Aug 30, 2017

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

ThomasBurleson Aug 31, 2017 • edited Loading

Choose a reason for hiding this comment

ThomasBurleson Aug 31, 2017 • edited Loading

Choose a reason for hiding this comment

ThomasBurleson Aug 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThomasBurleson commented Sep 1, 2017

mhevery commented Sep 1, 2017

0x-r4bbit commented Sep 7, 2017

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

ThomasBurleson commented Aug 24, 2017 •

edited

Loading

ThomasBurleson commented Aug 24, 2017 •

edited

Loading

ThomasBurleson Aug 31, 2017 •

edited

Loading

ThomasBurleson Aug 31, 2017 •

edited

Loading

ThomasBurleson Aug 31, 2017 •

edited

Loading