fix(ivy): ensure parent/sub-class components evaluate styling correctly by matsko · Pull Request #29602 · angular/angular

matsko · 2019-03-29T22:55:47Z

The new styling algorithm in angular is designed to evaluate host
bindings stylinh priority in order of directive evaluation order. This,
however, does not work with respect to parent/sub-class directives
because sub-class host bindings are run after the parent host bindings
but still have priority. This patch ensures that the host styling bindings
for parent and sub-class components/directives are executed with respect
to the styling algorithm prioritization.

Jira Issue: FW-1132

mhevery · 2019-04-03T03:47:43Z

I think increment/decrement be in try-finally?

Should incrementActiveHostDirectiveInheritanceDepth we merge incrementActiveHostDirectiveIndex since they are always called together?

mhevery · 2019-04-03T03:51:52Z

Suggested change

// since there can only one hostBindings function per root

// since there can only be one hostBindings function per root

mhevery · 2019-04-03T03:56:54Z

Is activeHostContext needed? Seems like a left over from previous iteration.

mhevery · 2019-04-03T03:57:50Z

if we reset depth to 0 here is decrementActiveHostDirectiveIndex method even needed?

mhevery · 2019-04-03T04:03:24Z

Do we need bothactiveHostDirectiveIndex and activeHostDirectiveInheritanceDepth they seem redundant.

Also I don't think activeHostDirectiveIndex is the right name. You are saying index which means it is index into something but than you just increment it. So it is more like uniqueId than on index?

mhevery · 2019-04-03T04:17:16Z

Suggested change

* A queue of all hostStyling* instructions.

* A queue of all hostStyling instructions.

Probably add a comment that the queue is only used for the host?

mhevery · 2019-04-03T04:17:30Z

Suggested change

* a queue is contructed and then flushed once the styles are applied to

* a queue is constructed and then flushed once the styles are applied to

mhevery · 2019-04-03T04:21:30Z

Suggested change

* at a later point (instead of immediately such as how templater-level styling

* at a later point (instead of immediately such as how template-level styling

mhevery · 2019-04-03T04:21:47Z

Suggested change

* directives, are evluated at different points (depending on priority) and will therefore

* directives, are evaluated at different points (depending on priority) and will therefore

mhevery · 2019-04-03T04:22:25Z

Suggested change

* Iterates throught the host instructions queue (if present within the provided

* Iterates through the host instructions queue (if present within the provided

pkozlowski-opensource · 2019-04-05T09:26:21Z

So the title of this test indicates that it is about styling instructions on <ng-container> but the setup of this test is much more complex and involves content projection? Is the content projection essential to reproduce the issue being fixed here? If yes, we should rename this test. If not, we should simplify it.

pkozlowski-opensource

@matsko I've left some comments (mostly about tests) that I would love to see addressed before this PR gets in (mostly nits so I hope it won't be too much trouble...). Thnx!

The new styling algorithm in angular is designed to evaluate host bindings stylinh priority in order of directive evaluation order. This, however, does not work with respect to parent/sub-class directives because sub-class host bindings are run after the parent host bindings but still have priority. This patch ensures that the host styling bindings for parent and sub-class components/directives are executed with respect to the styling algorithm prioritization. Jira Issue: FW-1132

matsko · 2019-04-05T18:01:33Z

https://tap.corp.google.com/ui#id=OCL:242073253:BASE:242154491:1554487206107:17aac872

…correctly

kara

A few more small changes

kara · 2019-04-05T21:07:07Z

+
+let activeDirectiveId = MIN_DIRECTIVE_ID;
+let activeDirectiveSuperClassDepthPosition = 0;
+let activeDirectiveSuperClassHeight = 0;


As far as I can tell, this global has no docs. It's not clear to me why it's necessary (in addition to the depth). Can you add docs that explain why it's necessary?

kara · 2019-04-05T21:13:46Z

-  const directiveIndex = getDirectiveIndexFromRegistry(context, directiveRef || null);
-
+    directiveIndex: number = 0): void {
+  assertValidDirectiveIndex(context, directiveIndex);


Seems like this should be an ngDevMode assert so it tree-shakes.

kara · 2019-04-05T21:33:03Z

-    directiveRef: any, forceOverride?: boolean): void {
-  const directiveIndex = getDirectiveIndexFromRegistry(context, directiveRef || null);
+    directiveIndex: number, forceOverride?: boolean): void {
+  assertValidDirectiveIndex(context, directiveIndex);


Needs devmode guard so it tree-shakes

kara

You'll probably need to re-run the symbol test now that assertValidDirectiveIndex should be tree shaken

kara · 2019-04-05T21:59:59Z

Suggested change

* Total count of how many directives are apart of an inheritance chain.

* Total count of how many directives are a part of an inheritance chain.

kara · 2019-04-05T22:00:06Z

Suggested change

* needs to keep track exactly how many were encountered so it can accurately

* needs to keep track of exactly how many were encountered so it can accurately

…correctly

…ly (angular#29602) The new styling algorithm in angular is designed to evaluate host bindings stylinh priority in order of directive evaluation order. This, however, does not work with respect to parent/sub-class directives because sub-class host bindings are run after the parent host bindings but still have priority. This patch ensures that the host styling bindings for parent and sub-class components/directives are executed with respect to the styling algorithm prioritization. Jira Issue: FW-1132 PR Close angular#29602

angular-automatic-lock-bot · 2019-09-14T22:51:49Z

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 Mar 29, 2019

jasonaden added the comp: ivy label Apr 1, 2019

ngbot Bot added this to the needsTriage milestone Apr 1, 2019

matsko force-pushed the styling_queue branch 7 times, most recently from d02327b to 54dce63 Compare April 2, 2019 23:43

matsko requested a review from mhevery April 2, 2019 23:44

matsko marked this pull request as ready for review April 2, 2019 23:45

matsko requested a review from a team April 2, 2019 23:45

matsko force-pushed the styling_queue branch from 54dce63 to 6f7b3e8 Compare April 3, 2019 00:35

matsko requested a review from a team April 3, 2019 00:35

mhevery suggested changes Apr 3, 2019

View reviewed changes

kara self-requested a review April 3, 2019 04:28

matsko force-pushed the styling_queue branch from 6f7b3e8 to f6f7454 Compare April 3, 2019 18:14

matsko requested a review from IgorMinar as a code owner April 3, 2019 18:14

matsko force-pushed the styling_queue branch 10 times, most recently from 5f8544e to 3293eb8 Compare April 4, 2019 00:07

gkalpak mentioned this pull request Apr 4, 2019

ci(docs-infra): run with Ivy #28530

Closed

12 of 16 tasks

pkozlowski-opensource reviewed Apr 5, 2019

View reviewed changes

pkozlowski-opensource requested changes Apr 5, 2019

View reviewed changes

matsko force-pushed the styling_queue branch 3 times, most recently from 5f91e95 to 5051180 Compare April 5, 2019 17:27

matsko force-pushed the styling_queue branch from 5051180 to 998fb09 Compare April 5, 2019 17:57

matsko requested review from kara, mhevery and pkozlowski-opensource and removed request for IgorMinar April 5, 2019 19:47

mhevery approved these changes Apr 5, 2019

View reviewed changes

pkozlowski-opensource approved these changes Apr 5, 2019

View reviewed changes

fixup! fix(ivy): ensure parent/sub-class components evaluate styling …

f54c708

…correctly

kara suggested changes Apr 5, 2019

View reviewed changes

matsko force-pushed the styling_queue branch 3 times, most recently from 50efa4e to a5155ab Compare April 5, 2019 21:56

kara approved these changes Apr 5, 2019

View reviewed changes

matsko force-pushed the styling_queue branch from a5155ab to 9520138 Compare April 5, 2019 22:09

fixup! fix(ivy): ensure parent/sub-class components evaluate styling …

311817d

…correctly

matsko force-pushed the styling_queue branch from 9520138 to 311817d Compare April 5, 2019 22:14

matsko added the action: merge The PR is ready for merge by the caretaker label Apr 5, 2019

IgorMinar closed this in ec56354 Apr 5, 2019

This was referenced Apr 6, 2019

Navigation memory usage increases on subsequent GulajavaMinistudio/angular#120

Merged

Navigation memory usage increases on subsequent TypescriptID/angular#320

Merged

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

	// since there can only one hostBindings function per root
	// since there can only be one hostBindings function per root

	* A queue of all hostStyling* instructions.
	* A queue of all hostStyling instructions.

	* a queue is contructed and then flushed once the styles are applied to
	* a queue is constructed and then flushed once the styles are applied to

	* at a later point (instead of immediately such as how templater-level styling
	* at a later point (instead of immediately such as how template-level styling

	* directives, are evluated at different points (depending on priority) and will therefore
	* directives, are evaluated at different points (depending on priority) and will therefore

	* Iterates throught the host instructions queue (if present within the provided
	* Iterates through the host instructions queue (if present within the provided

	* Total count of how many directives are apart of an inheritance chain.
	* Total count of how many directives are a part of an inheritance chain.

	* needs to keep track exactly how many were encountered so it can accurately
	* needs to keep track of exactly how many were encountered so it can accurately

Conversation

matsko commented Mar 29, 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pkozlowski-opensource left a comment

Choose a reason for hiding this comment

Uh oh!

matsko commented Apr 5, 2019

Uh oh!

kara 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!

kara Apr 5, 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!

kara 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!

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

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

kara Apr 5, 2019 •

edited

Loading