refactor(ivy): make styling instructions use the new styling algorithm #30742

matsko · 2019-05-30T02:53:07Z

This commit is the final patch of the ivy styling algorithm refactor.
This patch swaps functionality from the old styling mechanism to the
new refactored code by changing the instruction code the compiler
generates and by pointing the runtime instruction code to the new
styling algorithm.

matsko · 2019-06-20T07:45:26Z

https://test.corp.google.com/ui#id=OCL:254147799:BASE:254147805:1561016705188:e47b6a1a

alfaproject · 2019-06-20T09:33:44Z

packages/compiler-cli/test/compliance/r3_view_compiler_styling_spec.ts

@@ -1172,7 +1158,7 @@ describe('compiler compliance: styling', () => {
      expectEmit(result.source, template, 'Incorrect template');
    });

-    it('should generate override instructions for only single-level styling bindings when !important is present',
+    it('should generate overridVjwjwe instructions for only single-level styling bindings when !important is present',


alfaproject · 2019-06-20T09:35:01Z

packages/compiler-cli/test/compliance/r3_view_compiler_styling_spec.ts

        }
        if (rf & 2) {
          $r3$.ɵɵproperty("id", ctx.id, null, true);
          $r3$.ɵɵproperty("title", ctx.title, null, true);
+          $r3$.ɵɵstyleSanitizer($r3$.ɵɵdefaultStyleSanitizer);


If a sanitizer is not given explicitly, shouldn't it just use the default one?

This sounds reasonable to me, @matsko WDYT?

alfaproject · 2019-06-20T09:36:03Z

packages/compiler/src/render3/view/compiler.ts

@@ -586,12 +586,7 @@ function createHostBindingsFunction(
    name?: string): o.Expression|null {
  // Initialize hostVarsCount to number of bound host properties (interpolations illegal),
  // except 'style' and 'class' properties, since they should *not* allocate host var slots


I guess this comment is outdated now

alfaproject · 2019-06-20T09:38:50Z

packages/core/src/debug/debug_node.ts

@@ -324,62 +327,41 @@ class DebugElement__POST_R3__ extends DebugNode__POST_R3__ implements DebugEleme
  }

  get classes(): {[key: string]: boolean;} {
-    const classes: {[key: string]: boolean;} = {};
+    let classes !: {[key: string]: boolean};


Type should be: let classes: {[key: string]: boolean} | undefined;

But why not just return where it's set and avoid branching?

The reason for this is because the if statement below assigns a new map and if it overwrites the value then you end up having an empty map be used for no reason.

I mean this:

get classes(): {[key: string]: boolean} { const element = this.nativeElement; if (element) { const context = loadLContextFromNode(element); const lView = context.lView; const tData = lView[TVIEW].data; const tNode = tData[context.nodeIndex] as TNode; if (tNode.classes) { if (isStylingContext(tNode.classes)) { return new NodeStylingDebug(tNode.classes as TStylingContext, lView, true).values; } else { return stylingMapToStringMap(tNode.classes); } } } return {}; }

(removes 1 branch from the function)

Ah I see. Yes. Excellent contribution.

alfaproject · 2019-06-20T09:39:48Z

packages/core/src/debug/debug_node.ts

  }

  get styles(): {[key: string]: string | null;} {
-    const styles: {[key: string]: string | null;} = {};
+    let styles !: {[key: string]: string | null};


Type should be: let styles: {[key: string]: string | null} | undefined;

But why not just return where it's set and avoid branching?

same thing here

packages/core/src/render3/styling_next/bindings.ts

AndrewKushnir · 2019-06-29T18:12:51Z

packages/core/src/render3/styling_next/bindings.ts

+      // it's a container element or it's apart of a test
+      // environment that doesn't have styling. In either
+      // case it's safe not to apply styling to the element.
+      const nativeStyle = native.style;
      if (value) {


@matsko it looks like we may need to adjust this check to take into account cases like the one described in #31345, where the value here would be 0 (with the type of number). My guess is that we may also have empty strings here, so may be we should check for null here, like:

if (value !== null) {

It's not directly related to this PR, we can do it in a separate one once this PR lands.
Thank you.

Good find. I will fix. Thank you.

!= needs to be used in the event that the value is undefined.

packages/core/src/debug/debug_node.ts

packages/core/src/render3/instructions/lview_debug.ts

packages/core/src/render3/instructions/element.ts

packages/core/src/render3/styling_next/bindings.ts

packages/core/src/render3/styling_next/util.ts

packages/core/test/render3/instructions_spec.ts

packages/core/src/render3/styling_next/bindings.ts

packages/core/src/render3/styling_next/instructions.ts

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

packages/core/src/render3/styling_next/bindings.ts

packages/core/src/render3/instructions/lview_debug.ts

packages/core/test/acceptance/styling_spec.ts

packages/core/src/render3/instructions/class_map_interpolation.ts

packages/core/src/render3/instructions/element.ts

matsko · 2019-07-19T19:21:36Z

http://test/OCL:254147799:BASE:259015467:1563564083413:4de779e8

kara

LGTM

kara · 2019-07-19T19:35:35Z

@matsko Seeing a cyclic dependency issue in G3

packages/core/src/render3/util/view_utils.ts -> packages/core/public_api.ts -> 
packages/core/src/core.ts -> packages/core/src/metadata.ts -> 
packages/core/src/metadata/directives.ts -> packages/core/src/render3/jit/directive.ts -> 
packages/core/src/render3/jit/environment.ts -> packages/core/src/sanitization/sanitization.ts -> 
packages/core/src/render3/state.ts -> packages/core/src/render3/assert.ts -> 
packages/core/src/render3/util/view_utils.ts

Update: this failure is from master, not this PR. Seems like we'll need to get that patched/fixed before we can run a useful presubmit.

This commit is the final patch of the ivy styling algorithm refactor. This patch swaps functionality from the old styling mechanism to the new refactored code by changing the instruction code the compiler generates and by pointing the runtime instruction code to the new styling algorithm.

matsko · 2019-07-19T22:25:44Z

Presubmit = http://test/OCL:254147799:BASE:259049799:1563575125970:1bf971b6

Ivy Presubmit = https://test.corp.google.com/ui#id=OCL:254147799:BASE:259054129:1563576661262:be161d43

angular#30742) This commit is the final patch of the ivy styling algorithm refactor. This patch swaps functionality from the old styling mechanism to the new refactored code by changing the instruction code the compiler generates and by pointing the runtime instruction code to the new styling algorithm. PR Close angular#30742

angular-automatic-lock-bot · 2019-09-15T08:20:55Z

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 May 30, 2019

matsko force-pushed the styling_refactor_cleanup branch 3 times, most recently from edccdbf to e5b9bca Compare May 30, 2019 18:52

matsko requested a review from mhevery May 30, 2019 19:01

matsko force-pushed the styling_refactor_cleanup branch 3 times, most recently from 6d3f4b2 to c102bd8 Compare May 30, 2019 21:07

matsko added the comp: ivy label May 30, 2019

ngbot bot added this to the needsTriage milestone May 30, 2019

matsko force-pushed the styling_refactor_cleanup branch from c102bd8 to 1c4b94a Compare June 2, 2019 22:03

matsko added the target: major This PR is targeted for the next major release label Jun 2, 2019

matsko force-pushed the styling_refactor_cleanup branch 12 times, most recently from 92813f8 to 91bdf16 Compare June 20, 2019 06:51

alfaproject reviewed Jun 20, 2019

View reviewed changes

pkozlowski-opensource reviewed Jun 28, 2019

View reviewed changes

packages/core/src/render3/styling_next/bindings.ts Outdated Show resolved Hide resolved

matsko force-pushed the styling_refactor_cleanup branch from 73bef1b to 70eaea1 Compare June 28, 2019 18:08

AndrewKushnir reviewed Jun 29, 2019

View reviewed changes

kara reviewed Jul 2, 2019

View reviewed changes

matsko force-pushed the styling_refactor_cleanup branch 4 times, most recently from eceecbe to 675a2aa Compare July 11, 2019 23:38

kara reviewed Jul 15, 2019

View reviewed changes

matsko force-pushed the styling_refactor_cleanup branch 7 times, most recently from eaa6092 to db9fd9b Compare July 19, 2019 17:41

kara approved these changes Jul 19, 2019

View reviewed changes

matsko force-pushed the styling_refactor_cleanup branch from 76810b0 to 05a41b3 Compare July 19, 2019 22:24

kara unassigned matsko Jul 19, 2019

kara closed this in 9c954eb Jul 19, 2019

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

refactor(ivy): make styling instructions use the new styling algorithm #30742

refactor(ivy): make styling instructions use the new styling algorithm #30742

Uh oh!

Conversation

matsko commented May 30, 2019

Uh oh!

matsko commented Jun 20, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alfaproject Jun 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alfaproject Jun 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alfaproject Jun 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

matsko commented Jul 19, 2019

Uh oh!

kara left a comment

Choose a reason for hiding this comment

Uh oh!

kara commented Jul 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matsko commented Jul 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

alfaproject Jun 20, 2019 •

edited

Loading

alfaproject Jun 20, 2019 •

edited

Loading

alfaproject Jun 20, 2019 •

edited

Loading

kara commented Jul 19, 2019 •

edited

Loading

matsko commented Jul 19, 2019 •

edited

Loading