Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Transplant views refactor #33702

Closed
wants to merge 6 commits into from

Conversation

@mhevery
Copy link
Member

mhevery commented Nov 8, 2019

Based on #33644 (please ignore during review)

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix/Performance
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@mhevery mhevery requested review from angular/fw-core as code owners Nov 8, 2019
@googlebot googlebot added the cla: yes label Nov 8, 2019
@mhevery mhevery force-pushed the mhevery:transplant_views_refactor branch from 48256af to 31c1f2e Nov 9, 2019
myTmpl !: TemplateRef<any>;
name: string = 'world';
get nameWithLog() {
if (ivyEnabled && !getCheckNoChangesMode()) {

This comment has been minimized.

Copy link
@pkozlowski-opensource

pkozlowski-opensource Nov 11, 2019

Member

I would personally prefer that we write those tests without being ivy specific and without digging into ivy internals. Such tests are brittle and we are not sure that we've got 1:1 behaviour with VE.

Couldn't we assert that the both interpolations are updated in the moved embedded view?

This comment has been minimized.

Copy link
@mhevery

mhevery Nov 12, 2019

Author Member

Good point. I tried to do this, but I have failed. I believe that is because the two actually behave differently. I believe the way VE solves this is that when you project a transplanted view which is not on-push it will force the component to on-push. This is in contrast to this PR which CDs the embedded view twice. Once in Declared component (always)and once inInserted` only when marked dirty. Happy to discuss this more.

const declaredComponentLView = findComponentView(lView);
for (let i = 0; i < movedViews.length; i++) {
const movedLView = movedViews[i] !;
let parentLView = movedLView[PARENT];

This comment has been minimized.

Copy link
@pkozlowski-opensource

pkozlowski-opensource Nov 11, 2019

Member

Not sure why we've got the logic of getting a parent and while (isLContainer(parentLView)) here. Shouldn't findComponentView go up the embedded views?

As the very minimum it would be great to add more comments here as the logic here starts to be non-obvious.

This comment has been minimized.

Copy link
@mhevery

mhevery Nov 12, 2019

Author Member

I have refactored it to make it more clear. But the short of it is that findComponentView does not cross LContainer boundaries.

}
const insertedComponentLView = findComponentView(parentLView !);
const insertionIsOnPush =
(insertedComponentLView[FLAGS] & LViewFlags.CheckAlways) !== LViewFlags.CheckAlways;

This comment has been minimized.

Copy link
@pkozlowski-opensource

pkozlowski-opensource Nov 11, 2019

Member

We are not really determining insertionIsOnPush here but rather than it is not CheckAlways, right?

It is not clear to me if this is intentional but I think that we should add comments + tests to explain what happens depending on the insertion view flags (OnPush, OnPush + marked for check, detatched etc.)

This comment has been minimized.

Copy link
@mhevery

mhevery Nov 12, 2019

Author Member

We are not really determining insertionIsOnPush here but rather than it is not CheckAlways, right?

So we don't have on-push flag. The way we mark a view on-push is to set CheckAlways to false. I have updated the comment.

PS: We do have ManualOnPush but that is not the same as OnPush. It is only used for wether a click listener should auto-mark the view dirty.

@ngbot ngbot bot modified the milestone: needsTriage Nov 11, 2019
@mhevery mhevery force-pushed the mhevery:transplant_views_refactor branch from 81f8ebe to 0e68a2f Nov 12, 2019
@mhevery

This comment has been minimized.

Copy link
Member Author

mhevery commented Nov 12, 2019

@mhevery mhevery force-pushed the mhevery:transplant_views_refactor branch from 0e68a2f to cf92ede Nov 12, 2019
@kara kara self-requested a review Nov 12, 2019
@mhevery mhevery mentioned this pull request Nov 12, 2019
4 of 14 tasks complete
@mhevery mhevery force-pushed the mhevery:transplant_views_refactor branch from e46d7a2 to 21ed744 Nov 13, 2019
Copy link
Contributor

kara left a comment

Like the idea of only searching moved views if the flag is set! Some minor things below

packages/core/src/render3/interfaces/container.ts Outdated Show resolved Hide resolved
packages/core/src/render3/interfaces/container.ts Outdated Show resolved Hide resolved
packages/core/src/render3/interfaces/container.ts Outdated Show resolved Hide resolved
packages/core/src/render3/interfaces/container.ts Outdated Show resolved Hide resolved
packages/core/src/render3/interfaces/view.ts Outdated Show resolved Hide resolved
packages/core/src/render3/instructions/container.ts Outdated Show resolved Hide resolved
mhevery and others added 2 commits Nov 13, 2019
Co-Authored-By: Kara <kara@users.noreply.github.com>
Co-Authored-By: Kara <kara@users.noreply.github.com>
@googlebot

This comment has been minimized.

Copy link

googlebot commented Nov 13, 2019

All (the pull request submitter and all commit authors) CLAs are signed, but 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 by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: no and removed cla: yes labels Nov 13, 2019
@mhevery mhevery added cla: yes and removed cla: no labels Nov 13, 2019
@googlebot

This comment has been minimized.

Copy link

googlebot commented Nov 13, 2019

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.

Co-Authored-By: Kara <kara@users.noreply.github.com>
@googlebot

This comment has been minimized.

Copy link

googlebot commented Nov 14, 2019

All (the pull request submitter and all commit authors) CLAs are signed, but 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 by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: no and removed cla: yes labels Nov 14, 2019
@kara
kara approved these changes Nov 14, 2019
Copy link
Contributor

kara left a comment

LGTM

@kara kara added cla: yes and removed cla: no labels Nov 14, 2019
@googlebot

This comment has been minimized.

Copy link

googlebot commented Nov 14, 2019

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.

Co-Authored-By: Kara <kara@users.noreply.github.com>
@googlebot

This comment has been minimized.

Copy link

googlebot commented Nov 14, 2019

All (the pull request submitter and all commit authors) CLAs are signed, but 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 by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: no and removed cla: yes labels Nov 14, 2019
@mhevery mhevery added cla: yes and removed cla: no labels Nov 14, 2019
@googlebot

This comment has been minimized.

Copy link

googlebot commented Nov 14, 2019

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.

@mhevery mhevery mentioned this pull request Nov 14, 2019
4 of 14 tasks complete
@alxhub alxhub closed this in f4cdd35 Nov 14, 2019
alxhub added a commit that referenced this pull request Nov 14, 2019
alxhub added a commit to alxhub/angular that referenced this pull request Nov 14, 2019
petebacondarwin added a commit to petebacondarwin/angular that referenced this pull request Nov 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.