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

Before node for view #33627

Conversation

@pkozlowski-opensource
Copy link
Member

pkozlowski-opensource commented Nov 6, 2019

This is the fix for FW-1559 split into 2 commits:

  • tests for (most) cases I could think of (I think that there are more, but couldn't easily reproduce in tests, need more time on those);
  • the actual fix - the fix makes the tests pass but results in some code / logic duplication.

This PR is split into 2 commits so we can decide to only take tests or test + fix.

I believe that we should merge it as-is and do the refactoring in the follow-up PR(s).

@googlebot googlebot added the cla: yes label Nov 6, 2019
@ngbot ngbot bot added this to the needsTriage milestone Nov 6, 2019
@pkozlowski-opensource pkozlowski-opensource force-pushed the pkozlowski-opensource:before_node_for_view branch from 91f3a28 to 84d8832 Nov 6, 2019

if (tNodeType === TNodeType.Element) {
return getNativeByTNode(tNode, lView);
} else if (tNodeType === TNodeType.Container) {

This comment has been minimized.

Copy link
@mhevery

mhevery Nov 6, 2019

Member

There is one more use case. You can have an LContainer but the TNode type is a element. This is because any element can be upgraded into a container. I think we need a test case for that.

This comment has been minimized.

Copy link
@pkozlowski-opensource

pkozlowski-opensource Nov 6, 2019

Author Member

Sure, I can add a test but since views inserted on a VCRef asked on an element are inserted after the element, it shouldn't matter for finding the first node of a view.

In fact I wanted to have a deeper look at the case of a directive asking for a ViwContainerRef on a <ng-container> in a follow-up PR as I'm not sure what will happen for a directive that asks for a ViewContainerRef on a <ng-container>

This comment has been minimized.

Copy link
@pkozlowski-opensource

This comment has been minimized.

Copy link
@mhevery

mhevery Nov 6, 2019

Member

I see, Thank you.

@pkozlowski-opensource pkozlowski-opensource force-pushed the pkozlowski-opensource:before_node_for_view branch 3 times, most recently from b05ce00 to 2784ff8 Nov 6, 2019
@pkozlowski-opensource pkozlowski-opensource marked this pull request as ready for review Nov 6, 2019
@pkozlowski-opensource pkozlowski-opensource requested review from angular/fw-core as code owners Nov 6, 2019
@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

AndrewKushnir commented Nov 6, 2019

Started g3 presubmits to see which targets might be fixed by this PR:

Will run a global TAP tonight as well.

Update: Global TAP Presubmit with Ivy.

// cc @atscott

@mhevery
mhevery approved these changes Nov 6, 2019

if (tNodeType === TNodeType.Element) {
return getNativeByTNode(tNode, lView);
} else if (tNodeType === TNodeType.Container) {

This comment has been minimized.

Copy link
@mhevery

mhevery Nov 6, 2019

Member

I see, Thank you.

@ngbot

This comment has been minimized.

Copy link

ngbot bot commented Nov 6, 2019

I see that you just added the PR action: merge label, but the following checks are still failing:
    failure conflicts with base branch "master"
    pending forbidden labels detected: PR action: review
    pending 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.

@mhevery mhevery mentioned this pull request Nov 6, 2019
5 of 14 tasks complete
@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

AndrewKushnir commented Nov 7, 2019

FYI, VE and Ivy Presubmits (including global one) are successful 👍

@pkozlowski-opensource could you plz rebase this PR and resolve a conflict, so this PR is ready to merge? Also quick question: should it be merged to master and patch (it's marked for master only atm)?

Thank you.

@pkozlowski-opensource pkozlowski-opensource force-pushed the pkozlowski-opensource:before_node_for_view branch from 2784ff8 to 4ad57b4 Nov 7, 2019
@pkozlowski-opensource

This comment has been minimized.

Copy link
Member Author

pkozlowski-opensource commented Nov 7, 2019

@AndrewKushnir rebased

Also quick question: should it be merged to master and patch (it's marked for master only atm)?

We should get it for both 9.x and 10.x so I guess master & patch?

@atscott

This comment has been minimized.

Copy link
Contributor

atscott commented Nov 7, 2019

@AndrewKushnir is investigating some weird things we're seeing in presubmits. This should still be good to merge - he'll remove the blocked label when the investigation is finished.

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

AndrewKushnir commented Nov 7, 2019

Just confirmed that the numbers are fine. The "blocked" label is removed. Thank you.

@kara kara closed this in c57759f Nov 8, 2019
kara added a commit that referenced this pull request Nov 8, 2019
kara added a commit that referenced this pull request Nov 8, 2019
kara added a commit that referenced this pull request Nov 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.