FW-869: Refactor view container and view manipulations for embedded views and projections #29031

benlesh · 2019-02-28T16:58:32Z

Views were being added to containers in such a way that they were out of sync with the order their corresponding elements existed in the DOM (and in the templates). This caused issues with how query results would come back, specifically that they would be in seemingly arbitrary order based off of the order in which they were retrieved (and therefor set up). Another side effect of this is that change detection was processed in the aforementioned order.

The fix was to refactor the view container and view interaction APIs, and rework the logic where views were being added to the container and linked to one another.

This also switches from an "insert before" strategy to an "insert after" strategy for adding DOM nodes.

Before:

<span>inserted 1</span><span>inserted 2</span><!-- container -->

After:

<!-- container --><span>inserted 1</span><span>inserted 2</span>

This also uncovered an issue related to i18n that has been marked with fixmeIvy('FW-1112: ...') in this PR. (attn @ocombe)

Thanks to @pkozlowski-opensource for the rapid assist with the query-related issue, @mhevery's guidance/ideas and @Combe for helping track down what was going on with the i18n test marked for fixing.

Update (2019-03-07) --------------

Everything was rebased as of last night at 8pm PST.

Two remaining known issue:

There are a few tests marked fixmeIvy('FW-????: ....'), I plan on replacing that ???? with an actual Jira number when this PR is closer to landing.
I have asked @ocombe to look into the remaining failing tests for i18n.

Because this is a large refactor, it's going to take a while to review and refine, so I'd like to get started with reviews prior to the above remaining issues being resolved.

googlebot · 2019-02-28T16:58:38Z

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 all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

googlebot · 2019-02-28T16:59:36Z

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

ℹ️ Googlers: Go here for more info.

googlebot · 2019-03-01T16:55:26Z

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 all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

googlebot · 2019-03-07T03:04:34Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

packages/core/src/render3/view.ts

packages/core/src/render3/component.ts

packages/core/src/render3/component_ref.ts

packages/core/src/render3/i18n.ts

packages/core/src/render3/instructions.ts

packages/core/test/render3/content_spec.ts

googlebot · 2019-03-08T14:20:28Z

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 all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

googlebot · 2019-03-08T19:04:29Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

kara

This is a pretty beefy PR, so this is my first attempt. General approach looks good. High level things to start with would probably be:

Split out addition of View interface into a separate PR (can be landed before this)
Split out unrelated docs changes (e.g. in i18n) into a separate PR (can be landed before this)
Split out new public API methods (e.g. getViewContainer) into a separate PR (can be landed after this)
Convert tests with handwritten generated code to TestBed
Audit uses of NATIVE vs HOST (noticed that they are used interchangeably)

packages/core/src/linker/query_list.ts

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

packages/core/src/render3/query.ts

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

packages/core/src/render3/view.ts

packages/core/src/render3/instructions.ts

kara · 2019-03-14T22:19:19Z

packages/core/src/render3/node_manipulation.ts

+  ngDevMode && assertLView(parentLView);
+  ngDevMode && assertLContainer(lContainerToAdd);
+
+  const anchorElement = lContainerToAdd[HOST];


If you want the host node, you'll need to unwrap it to avoid getting a component view or a styling context.

Suggested change

const anchorElement = lContainerToAdd[HOST];

const anchorElement = unwrapRNode(lContainerToAdd);

Which begs the question - do you want the host node? Or are you looking specifically for a comment? In that case, it should be NATIVE.

I've switched to NATIVE, because I'm sure that's what I really wanted... however it does concern me that no tests broke from me using HOST instead.

cc @mhevery

packages/core/src/render3/node_manipulation.ts

packages/core/src/render3/view.ts

… INTO SEPERATE PRS

@mhevery

Just updating comments in query-related things to make it easier for the next person that has to grok this for the first time. Also adds a demo from @mhevery to one of the query specs Related angular#29031

@mhevery

Just updating comments in query-related things to make it easier for the next person that has to grok this for the first time. Also adds a demo from @mhevery to one of the query specs Related #29031 PR Close #29342

@mhevery

Just updating comments in query-related things to make it easier for the next person that has to grok this for the first time. Also adds a demo from @mhevery to one of the query specs Related angular#29031

@mhevery

Just updating comments in query-related things to make it easier for the next person that has to grok this for the first time. Also adds a demo from @mhevery to one of the query specs Related #29031 PR Close #29342

benlesh · 2019-03-20T19:06:41Z

Closing this as it's replaced by #29339 and #29340. Although I'll be back to review the comments for sure.

@mhevery

Just updating comments in query-related things to make it easier for the next person that has to grok this for the first time. Also adds a demo from @mhevery to one of the query specs Related angular#29031 PR Close angular#29342

@mhevery

Just updating comments in query-related things to make it easier for the next person that has to grok this for the first time. Also adds a demo from @mhevery to one of the query specs Related angular#29031 PR Close angular#29342

angular-automatic-lock-bot · 2019-09-14T20:52:36Z

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: no label Feb 28, 2019

benlesh added cla: yes area: core Issues related to the framework runtime target: major This PR is targeted for the next major release comp: ivy risk: medium labels Feb 28, 2019

ngbot bot added this to the needsTriage milestone Feb 28, 2019

benlesh removed the cla: no label Feb 28, 2019

googlebot added cla: no and removed cla: yes labels Mar 1, 2019

benlesh force-pushed the FW-869-start-two branch from 815233f to 2e5be9c Compare March 7, 2019 03:04

googlebot added cla: yes and removed cla: no labels Mar 7, 2019

benlesh force-pushed the FW-869-start-two branch from 2e5be9c to 0a097f6 Compare March 7, 2019 04:34

mhevery suggested changes Mar 8, 2019

View reviewed changes

googlebot added cla: no and removed cla: yes labels Mar 8, 2019

benlesh force-pushed the FW-869-start-two branch from f080730 to f06a997 Compare March 8, 2019 19:04

googlebot added cla: yes and removed cla: no labels Mar 8, 2019

benlesh force-pushed the FW-869-start-two branch 4 times, most recently from 7fafe5e to f3f6070 Compare March 12, 2019 23:36

benlesh force-pushed the FW-869-start-two branch 3 times, most recently from 8abede8 to ff720de Compare March 14, 2019 17:00

kara requested changes Mar 14, 2019

View reviewed changes

benlesh force-pushed the FW-869-start-two branch from b65c9c5 to 4c2ca31 Compare March 15, 2019 18:23

refactor(ivy): insert after instead of before, COMMIT TO BE BROKEN UP…

468e813

… INTO SEPERATE PRS

benlesh force-pushed the FW-869-start-two branch from 4c2ca31 to 468e813 Compare March 15, 2019 18:27

benlesh mentioned this pull request Mar 15, 2019

Insert after comment #29340

Closed

benlesh mentioned this pull request Mar 15, 2019

refactor(ivy): Update query-related comments #29342

Closed

benlesh requested a review from kara March 16, 2019 00:32

benlesh mentioned this pull request Mar 18, 2019

refactor(ivy): add new view container api #29339

Closed

benlesh closed this Mar 20, 2019

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

FW-869: Refactor view container and view manipulations for embedded views and projections #29031

FW-869: Refactor view container and view manipulations for embedded views and projections #29031

benlesh commented Feb 28, 2019 •

edited

googlebot commented Feb 28, 2019

googlebot commented Feb 28, 2019

googlebot commented Mar 1, 2019

googlebot commented Mar 7, 2019

googlebot commented Mar 8, 2019

googlebot commented Mar 8, 2019

kara left a comment •

edited

kara Mar 14, 2019 •

edited

benlesh Mar 15, 2019

benlesh commented Mar 20, 2019

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

	const anchorElement = lContainerToAdd[HOST];
	const anchorElement = unwrapRNode(lContainerToAdd);

FW-869: Refactor view container and view manipulations for embedded views and projections #29031

FW-869: Refactor view container and view manipulations for embedded views and projections #29031

Conversation

benlesh commented Feb 28, 2019 • edited

Update (2019-03-07) --------------

googlebot commented Feb 28, 2019

googlebot commented Feb 28, 2019

googlebot commented Mar 1, 2019

googlebot commented Mar 7, 2019

googlebot commented Mar 8, 2019

googlebot commented Mar 8, 2019

kara left a comment • edited

Choose a reason for hiding this comment

kara Mar 14, 2019 • edited

Choose a reason for hiding this comment

benlesh Mar 15, 2019

Choose a reason for hiding this comment

benlesh commented Mar 20, 2019

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

benlesh commented Feb 28, 2019 •

edited

kara left a comment •

edited

kara Mar 14, 2019 •

edited