refactor(ivy): LView is a proper linked list #28382

benlesh · 2019-01-25T23:22:32Z

TView no longer stores childIndex
LView now as CHILD_HEAD and CHILD_TAIL

TView used to store the head of the list, therefor all LViews had to have the same head, which is incorrect.

kara

Looks good, but needs a test for the new behavior we want to support (e.g. using injector.get to get ViewContainerRef.

Also needs symbol test updates and linting.

packages/core/src/render3/instructions.ts

gkalpak

Isn't it possible to also remove views? Shouldn't CHILD_HEAD/CHILD_TAIL be updatd in that case?

packages/core/src/render3/node_manipulation.ts

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

packages/core/src/render3/instructions.ts

benlesh · 2019-02-04T19:05:39Z

presubmit: https://test.corp.google.com/ui#id=OCL:232328029:BASE:232328145:1549306899223:9eaec735

kara

Nice work, like how much more intuitive the data structures are.

packages/core/src/render3/assert.ts

packages/core/src/render3/discovery_utils.ts

packages/core/src/render3/instructions.ts

packages/core/src/render3/node_manipulation.ts

packages/core/src/render3/util.ts

packages/core/src/render3/discovery_utils.ts

kara · 2019-02-14T19:30:41Z

packages/core/src/render3/instructions.ts

+export function addToViewTree<T extends LView|LContainer>(lView: LView, lViewOrLContainer: T): T {
+  // TODO(benlesh/misko): This implementation is incorrect, because it always adds the LContainer to
+  // the end of the queue, which means if the developer asks for the LContainers out of order, the
+  // change detection will run out of order.


^ Still needs resolution

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

benlesh · 2019-02-19T18:00:57Z

@kara The TODO in addViewToTree is actually addressed in another PR that's in-progress (for FW-869). So that will not be resolved in this PR.

kara · 2019-02-19T18:18:48Z

@benlesh To clarify, the feedback that still needed resolution was that we need a better comment. I don't see why improving the comment needs to wait until the next PR.

Maybe add clarification to comment about what "asks for" means? Do you mean injector.get(ViewContainerRef)? (Also is there a Jira ticket for this?)

IgorMinar · 2019-02-20T00:19:51Z

google3 presumit fails... please update and remove the "cleanup" label when ready

benlesh · 2019-02-21T22:40:36Z

new presubmit

packages/core/src/debug/debug_node.ts

- Removes CONTAINER_INDEX - LView[PARENT] now contains LContainer when necessary - Removes now unused arguments to methods after refactor

- TView no longer stores childIndex - LView now as CHILD_HEAD and CHILD_TAIL TView used to store the head of the list, therefor all LViews had to have the same head, which is incorrect.

Google3 detected circular references here, so splitting up this rather hodge-podge list of functions into slightly better organizational units.

IgorMinar · 2019-02-22T20:16:03Z

this pr still fails on g3 presubmit. adding "cleanup" label.

root cause:

failed to load main  Error: Cyclical dependency between files:

/build/work/3b02cb54a0edf0b2b24d9ca8ccb8de16/google3/third_party/javascript/angular2/rc/packages/core/src/render3/util/view_utils.ts -> 
/build/work/3b02cb54a0edf0b2b24d9ca8ccb8de16/google3/third_party/javascript/angular2/rc/packages/core/public_api.ts -> 
/build/work/3b02cb54a0edf0b2b24d9ca8ccb8de16/google3/third_party/javascript/angular2/rc/packages/core/src/core.ts -> 
/build/work/3b02cb54a0edf0b2b24d9ca8ccb8de16/google3/third_party/javascript/angular2/rc/packages/core/src/metadata.ts -> 
/build/work/3b02cb54a0edf0b2b24d9ca8ccb8de16/google3/third_party/javascript/angular2/rc/packages/core/src/metadata/directives.ts -> 
/build/work/3b02cb54a0edf0b2b24d9ca8ccb8de16/google3/third_party/javascript/angular2/rc/packages/core/src/render3/jit/directive.ts -> 
/build/work/3b02cb54a0edf0b2b24d9ca8ccb8de16/google3/third_party/javascript/angular2/rc/packages/core/src/render3/util/misc_utils.ts -> 
/build/work/3b02cb54a0edf0b2b24d9ca8ccb8de16/google3/third_party/javascript/angular2/rc/packages/core/src/render3/util/view_traversal_utils.ts -> 
/build/work/3b02cb54a0edf0b2b24d9ca8ccb8de16/google3/third_party/javascript/angular2/rc/packages/core/src/render3/assert.ts -> 
/build/work/3b02cb54a0edf0b2b24d9ca8ccb8de16/google3/third_party/javascript/angular2/rc/packages/core/src/render3/util/view_utils.ts

benlesh · 2019-02-22T21:14:31Z

@IgorMinar that's an old presubmit, it's passing now:

https://test.corp.google.com/ui#id=OCL:232328029:BASE:235220976:1550861966438:b7aecde3

- TView no longer stores childIndex - LView now as CHILD_HEAD and CHILD_TAIL TView used to store the head of the list, therefor all LViews had to have the same head, which is incorrect. PR Close #28382

PR Close #28382

Google3 detected circular references here, so splitting up this rather hodge-podge list of functions into slightly better organizational units. PR Close #28382

angular-automatic-lock-bot · 2019-09-14T17:53:11Z

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

benlesh requested a review from kara January 25, 2019 23:22

benlesh requested a review from a team as a code owner January 25, 2019 23:22

googlebot added the cla: yes label Jan 25, 2019

benlesh added target: major This PR is targeted for the next major release comp: ivy and removed cla: yes labels Jan 25, 2019

googlebot added the cla: yes label Jan 25, 2019

benlesh added risk: low and removed cla: yes labels Jan 25, 2019

googlebot added the cla: yes label Jan 25, 2019

ngbot bot added this to the needsTriage milestone Jan 25, 2019

kara requested changes Jan 25, 2019

View reviewed changes

packages/core/src/render3/instructions.ts Show resolved Hide resolved

kara 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 Jan 25, 2019

kara assigned benlesh Jan 25, 2019

benlesh force-pushed the move-lcontainer-head-to-lview branch from 9d3e2e3 to c3b5066 Compare January 29, 2019 00:01

gkalpak reviewed Jan 29, 2019

View reviewed changes

packages/core/src/render3/node_manipulation.ts Outdated Show resolved Hide resolved

packages/core/src/render3/interfaces/view.ts Outdated Show resolved Hide resolved

benlesh force-pushed the move-lcontainer-head-to-lview branch 2 times, most recently from b6e7747 to 213690f Compare February 4, 2019 17:02

AndrewKushnir reviewed Feb 4, 2019

View reviewed changes

packages/core/src/render3/instructions.ts Outdated Show resolved Hide resolved

benlesh 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 Feb 4, 2019

kara requested changes Feb 8, 2019

View reviewed changes

benlesh force-pushed the move-lcontainer-head-to-lview branch 5 times, most recently from b2c80e7 to f4b407b Compare February 14, 2019 19:12

kara requested changes Feb 14, 2019

View reviewed changes

IgorMinar 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 Feb 20, 2019

benlesh removed the action: merge The PR is ready for merge by the caretaker label Feb 20, 2019

benlesh requested a review from a team as a code owner February 20, 2019 22:21

benlesh force-pushed the move-lcontainer-head-to-lview branch from 9624296 to a43b0bc Compare February 21, 2019 01:15

benlesh requested a review from a team as a code owner February 21, 2019 01:15

benlesh force-pushed the move-lcontainer-head-to-lview branch from a43b0bc to f6370e1 Compare February 21, 2019 18:13

benlesh 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 Feb 21, 2019

AndrewKushnir reviewed Feb 22, 2019

View reviewed changes

packages/core/src/debug/debug_node.ts Show resolved Hide resolved

benlesh force-pushed the move-lcontainer-head-to-lview branch from 7b59f8f to a9a69e7 Compare February 22, 2019 18:34

benlesh added 3 commits February 22, 2019 10:43

refactor(ivy): LContainer now stored in LView[PARENT]

e4c0c14

- Removes CONTAINER_INDEX - LView[PARENT] now contains LContainer when necessary - Removes now unused arguments to methods after refactor

refactor(ivy): LView is a proper linked list

2d702a4

- TView no longer stores childIndex - LView now as CHILD_HEAD and CHILD_TAIL TView used to store the head of the list, therefor all LViews had to have the same head, which is incorrect.

fix(ivy): assertDomNode will now work properly in a worker

21f9ab6

benlesh force-pushed the move-lcontainer-head-to-lview branch from a9a69e7 to 71494ea Compare February 22, 2019 18:46

refactor(ivy): split util functions into different files

3f633a1

Google3 detected circular references here, so splitting up this rather hodge-podge list of functions into slightly better organizational units.

benlesh force-pushed the move-lcontainer-head-to-lview branch from 71494ea to 3f633a1 Compare February 22, 2019 18:58

benlesh added action: merge The PR is ready for merge by the caretaker and removed merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels Feb 22, 2019

IgorMinar 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 Feb 22, 2019

mhevery approved these changes Feb 22, 2019

View reviewed changes

benlesh 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 Feb 22, 2019

benlesh closed this in ba6aa93 Feb 22, 2019

benlesh added a commit that referenced this pull request Feb 22, 2019

fix(ivy): assertDomNode will now work properly in a worker (#28382)

7bae49b

PR Close #28382

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

refactor(ivy): LView is a proper linked list #28382

refactor(ivy): LView is a proper linked list #28382

benlesh commented Jan 25, 2019 •

edited

kara left a comment •

edited

gkalpak left a comment

benlesh commented Feb 4, 2019

kara left a comment

kara Feb 14, 2019

benlesh commented Feb 19, 2019

kara commented Feb 19, 2019

IgorMinar commented Feb 20, 2019

benlesh commented Feb 21, 2019

IgorMinar commented Feb 22, 2019 •

edited

benlesh commented Feb 22, 2019

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

refactor(ivy): LView is a proper linked list #28382

refactor(ivy): LView is a proper linked list #28382

Conversation

benlesh commented Jan 25, 2019 • edited

kara left a comment • edited

Choose a reason for hiding this comment

gkalpak left a comment

Choose a reason for hiding this comment

benlesh commented Feb 4, 2019

kara left a comment

Choose a reason for hiding this comment

kara Feb 14, 2019

Choose a reason for hiding this comment

benlesh commented Feb 19, 2019

kara commented Feb 19, 2019

IgorMinar commented Feb 20, 2019

benlesh commented Feb 21, 2019

IgorMinar commented Feb 22, 2019 • edited

benlesh commented Feb 22, 2019

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

benlesh commented Jan 25, 2019 •

edited

kara left a comment •

edited

IgorMinar commented Feb 22, 2019 •

edited