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

fix(ivy): error if directive with synthetic property binding is on same node as directive that injects ViewContainerRef #35343

Closed
wants to merge 1 commit into from

Conversation

crisbeto
Copy link
Member

In the loadRenderer we make an assumption that the value will always be an LView, but if there's a directive on the same node which injects ViewContainerRef the LView will be wrapped in an LContainer. These changes add a call to unwrap the value before we try to read the value off of it.

Fixes #35342.

…me node as directive that injects ViewContainerRef

In the `loadRenderer` we make an assumption that the value will always be an `LView`, but if there's a directive on the same node which injects `ViewContainerRef` the `LView` will be wrapped in an `LContainer`. These changes add a call to unwrap the value before we try to read the value off of it.

Fixes angular#35342.
@crisbeto crisbeto added comp: ivy action: review The PR is still awaiting reviews from at least one requested reviewer target: patch This PR is targeted for the next patch release labels Feb 11, 2020
@ngbot ngbot bot modified the milestone: needsTriage Feb 11, 2020
@crisbeto crisbeto marked this pull request as ready for review February 11, 2020 19:23
expect(() => {
const fixture = TestBed.createComponent(App);
fixture.detectChanges();
}).not.toThrow();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that you need explicit not.toThrow(); a test would fail in case of an exception thrown.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually prefer this as it makes it very clear what you are testing. Without this it is harder to understand what exactly you are testing. I think of it as a marker.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, I hear what you say about marker and this is a good point. The problem I've got with such assertion, though, is that this test is going to pass even if nothing happens / gets rendered. I could pretty much delete the entire template and the test would still pass.

This is why, personally, I prefer to have a more concrete assert... Definitively don't want to split a hair over this one, just sharing personal preference / how I think about it...

@mhevery
Copy link
Contributor

mhevery commented Feb 11, 2020

presubmit

@mhevery mhevery added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Feb 11, 2020
@kara kara closed this in d6bc63f Feb 13, 2020
kara pushed a commit that referenced this pull request Feb 13, 2020
…me node as directive that injects ViewContainerRef (#35343)

In the `loadRenderer` we make an assumption that the value will always be an `LView`, but if there's a directive on the same node which injects `ViewContainerRef` the `LView` will be wrapped in an `LContainer`. These changes add a call to unwrap the value before we try to read the value off of it.

Fixes #35342.

PR Close #35343
sonukapoor pushed a commit to sonukapoor/angular that referenced this pull request Feb 17, 2020
…me node as directive that injects ViewContainerRef (angular#35343)

In the `loadRenderer` we make an assumption that the value will always be an `LView`, but if there's a directive on the same node which injects `ViewContainerRef` the `LView` will be wrapped in an `LContainer`. These changes add a call to unwrap the value before we try to read the value off of it.

Fixes angular#35342.

PR Close angular#35343
@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Mar 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't attach a matTooltip to mat-expansion-panel-header since 9.0.0
4 participants