fix(ivy): special case [style] and [class] bindings for future use #23232

matsko · 2018-04-06T20:41:29Z

No description provided.

mary-poppins · 2018-04-06T20:50:52Z

You can preview 749ed3f at https://pr23232-749ed3f.ngbuilds.io/.

mhevery

one small nit, LGTM otherwise

mhevery · 2018-04-06T20:59:01Z

packages/compiler/test/render3/r3_view_compiler_styling_spec.ts

+import {MockDirectory, setup} from '../aot/test_util';
+import {compile, expectEmit} from './mock_compile';
+
+/* These tests are codified version of the tests in compiler_canonical_spec.ts. Every


remove this comment as it is not relevant

vicb · 2018-04-06T22:56:25Z

packages/compiler/src/render3/r3_view_compiler.ts

+            convertedBinding);
+        continue;
+      }
+
      const instruction = BINDING_INSTRUCTION_MAP[input.type];


why not const instruction = getInputInstruction(input); ?

(or something similar as the number of args is different, the idea being to centralize the computation of the instruction)

actually something like

// class & style const specialInstruction = SPECIAL_BINDING_INSTRUCTION_MAP[input.name]; if (specialInstruction) { this.instruction( this._bindingMode, input.sourceSpan, specialInstruction, o.literal(elementIndex), convertedBinding); continue; } const instruction = ...; // ...

vicb · 2018-04-06T22:58:22Z

packages/compiler/test/render3/r3_view_compiler_styling_spec.ts

+              $r3$.ɵE(0, 'div');
+              $r3$.ɵe();
+            }
+            $r3$.ɵs(0,ctx.myStyleExp);


ctx should probably not be hardcoded -> use $ctx$ instead ? (in the arg list and here)

vicb

see inline comments

mary-poppins · 2018-04-07T23:01:50Z

You can preview 5e61db0 at https://pr23232-5e61db0.ngbuilds.io/.

mary-poppins · 2018-04-07T23:04:02Z

You can preview 5c4fc09 at https://pr23232-5c4fc09.ngbuilds.io/.

mary-poppins · 2018-04-07T23:07:12Z

You can preview 3c1d545 at https://pr23232-3c1d545.ngbuilds.io/.

sandangel · 2018-04-07T23:13:00Z

packages/compiler/test/render3/r3_view_compiler_styling_spec.ts

+              $r3$.ɵE(0, 'div');
+              $r3$.ɵe();
+            }
+            $r3$.ɵs(0,$ctx$.myStyleExp);


space after comma?

No it's not needed because this code is generated by AOT (and it conserves size by avoiding unnecessary space usage).

this is a test, and spaces are ignored so for readability you should add it.

sandangel · 2018-04-07T23:13:11Z

packages/compiler/test/render3/r3_view_compiler_styling_spec.ts

+              $r3$.ɵE(0, 'div');
+              $r3$.ɵe();
+            }
+            $r3$.ɵk(0,$ctx$.myClassExp);


space after comma?

See comment above.

see my comment above. please add space to make it more readable. This is test only and will not have effect on output of compiler.

vicb · 2018-04-09T16:31:36Z

packages/compiler/src/render3/r3_view_compiler.ts

@@ -267,6 +267,11 @@ const BINDING_INSTRUCTION_MAP: {[index: number]: o.ExternalReference | undefined
  [PropertyBindingType.Style]: R3.elementStyleNamed
 };

+const SPECIAL_CASED_BINDING_INSTRUCTION_MAP: {[index: string]: o.ExternalReference | undefined} = {


| undefined ?

vicb

inline comments

vicb · 2018-04-12T18:48:27Z

packages/compiler/src/render3/r3_view_compiler.ts

@@ -273,6 +273,11 @@ const BINDING_INSTRUCTION_MAP: {[index: number]: o.ExternalReference | undefined
  [PropertyBindingType.Style]: R3.elementStyleNamed
 };

+const SPECIAL_CASED_BINDING_INSTRUCTION_MAP: {[index: string]: o.ExternalReference} = {
+  'className': R3.elementClass,


vicb · 2018-04-12T18:48:56Z

packages/compiler/test/render3/r3_view_compiler_styling_spec.ts

+import {MockDirectory, setup} from '../aot/test_util';
+import {compile, expectEmit} from './mock_compile';
+
+/* These tests are codified version of the tests in compiler_canonical_spec.ts. Every


mary-poppins · 2018-04-12T18:59:53Z

You can preview 29d4466 at https://pr23232-29d4466.ngbuilds.io/.

mary-poppins · 2018-04-12T19:08:34Z

You can preview 80feb9d at https://pr23232-80feb9d.ngbuilds.io/.

ngbot · 2018-04-23T17:25:10Z

Hi @matsko! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

mary-poppins · 2018-06-01T16:49:15Z

You can preview e1d89d9 at https://pr23232-e1d89d9.ngbuilds.io/.

mary-poppins · 2018-06-01T23:38:34Z

You can preview f0ca8b9 at https://pr23232-f0ca8b9.ngbuilds.io/.

JoostK · 2018-06-04T19:20:59Z

packages/compiler/test/render3/r3_view_compiler_styling_spec.ts

+              $r3$.ɵe();
+            }
+            if (rf & 2) {
+              $r3$.ɵs(0,$r3$.ɵb($ctx$.myClassExp));


Shouldn't this be ɵk?

mary-poppins · 2018-06-05T19:01:14Z

You can preview 9e7e278 at https://pr23232-9e7e278.ngbuilds.io/.

ngbot · 2018-06-06T17:45:07Z

I see that you just added the PR action: merge label, but the following checks are still failing:
missing required status "code-review/pullapprove"
status "google3" is pending

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

vicb

please update status.md

mary-poppins · 2018-06-07T18:39:03Z

You can preview e284c46 at https://pr23232-e284c46.ngbuilds.io/.

I have updated the STATUS.md

mhevery · 2018-06-07T21:01:38Z

STATUS.md updated

… use

googlebot · 2018-06-07T21:05:43Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

googlebot · 2018-06-07T21:07:54Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

mary-poppins · 2018-06-07T21:17:36Z

You can preview 292f37d at https://pr23232-292f37d.ngbuilds.io/.

vicb · 2018-06-08T16:04:01Z

packages/compiler/src/render3/r3_identifiers.ts

@@ -25,8 +25,12 @@ export class Identifiers {

  static elementAttribute: o.ExternalReference = {name: 'ɵa', moduleName: CORE};

+  static elementClass: o.ExternalReference = {name: 'ɵk', moduleName: CORE};


Could you please update environment.ts with that.

Also may be add a comment in this file and environment.ts to remember to keep them in sync.

Thanks

Edit: The instructions are actually already there.

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

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 Apr 6, 2018

matsko requested a review from mhevery April 6, 2018 20:41

matsko added the target: major This PR is targeted for the next major release label Apr 6, 2018

mhevery approved these changes Apr 6, 2018

View reviewed changes

vicb reviewed Apr 6, 2018

View reviewed changes

vicb suggested changes Apr 6, 2018

View reviewed changes

matsko force-pushed the ivy_style_property branch from 749ed3f to 5e61db0 Compare April 7, 2018 22:52

matsko changed the title ~~Special case [style] and [class] bindings~~ fix(ivy): special case [style] and [class] bindings for future use Apr 7, 2018

matsko force-pushed the ivy_style_property branch 2 times, most recently from 5c4fc09 to 3c1d545 Compare April 7, 2018 22:55

sandangel reviewed Apr 7, 2018

View reviewed changes

vicb reviewed Apr 9, 2018

View reviewed changes

matsko force-pushed the ivy_style_property branch from 3c1d545 to 29d4466 Compare April 12, 2018 18:46

vicb reviewed Apr 12, 2018

View reviewed changes

matsko force-pushed the ivy_style_property branch from 29d4466 to 80feb9d Compare April 12, 2018 18:58

vicb approved these changes Apr 12, 2018

View reviewed changes

jasonaden added the comp: ivy label Apr 13, 2018

matsko force-pushed the ivy_style_property branch from 80feb9d to e1d89d9 Compare June 1, 2018 16:38

matsko force-pushed the ivy_style_property branch from e1d89d9 to f0ca8b9 Compare June 1, 2018 23:30

JoostK reviewed Jun 4, 2018

View reviewed changes

matsko force-pushed the ivy_style_property branch from 428f29d to 9e7e278 Compare June 5, 2018 18:43

matsko added the action: merge The PR is ready for merge by the caretaker label Jun 6, 2018

matsko requested review from mhevery and removed request for mhevery June 6, 2018 20:30

vicb previously requested changes Jun 6, 2018

View reviewed changes

vicb 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 7, 2018

fix(ivy): special case [style] and [class] bindings for future use

e284c46

matsko force-pushed the ivy_style_property branch from 9e7e278 to e284c46 Compare June 7, 2018 18:24

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

fixup! fix(ivy): special case [style] and [class] bindings for future…

292f37d

… use

googlebot added cla: no and removed cla: yes labels Jun 7, 2018

mhevery added cla: yes and removed cla: no labels Jun 7, 2018

vicb reviewed Jun 8, 2018

View reviewed changes

vicb approved these changes Jun 8, 2018

View reviewed changes

mhevery closed this in 1b253e1 Jun 8, 2018

matsko deleted the ivy_style_property branch January 17, 2019 19:44

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

		@@ -25,8 +25,12 @@ export class Identifiers {

		static elementAttribute: o.ExternalReference = {name: 'ɵa', moduleName: CORE};

		static elementClass: o.ExternalReference = {name: 'ɵk', moduleName: CORE};

fix(ivy): special case [style] and [class] bindings for future use #23232

fix(ivy): special case [style] and [class] bindings for future use #23232

Uh oh!

Conversation

matsko commented Apr 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mary-poppins commented Apr 6, 2018

Uh oh!

mhevery left a comment

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!

vicb Apr 6, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

vicb left a comment

Choose a reason for hiding this comment

Uh oh!

mary-poppins commented Apr 7, 2018

Uh oh!

mary-poppins commented Apr 7, 2018

Uh oh!

mary-poppins commented Apr 7, 2018

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!

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!

vicb left a comment

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!

mary-poppins commented Apr 12, 2018

Uh oh!

mary-poppins commented Apr 12, 2018

Uh oh!

ngbot bot commented Apr 23, 2018

Uh oh!

mary-poppins commented Jun 1, 2018

Uh oh!

mary-poppins commented Jun 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mary-poppins commented Jun 5, 2018

Uh oh!

ngbot bot commented Jun 6, 2018

Uh oh!

vicb left a comment

Choose a reason for hiding this comment

Uh oh!

mary-poppins commented Jun 7, 2018

Uh oh!

mhevery commented Jun 7, 2018

Uh oh!

googlebot commented Jun 7, 2018

Uh oh!

googlebot commented Jun 7, 2018

matsko commented Apr 6, 2018 •

edited

Loading

vicb Apr 6, 2018 •

edited

Loading

vicb Jun 8, 2018 •

edited

Loading