Skip to content

fix(ivy): removing false-negative assert in getOrCreateNodeInjector function - #26305

Closed
AndrewKushnir wants to merge 5 commits into
angular:masterfrom
AndrewKushnir:di-assert-rm
Closed

fix(ivy): removing false-negative assert in getOrCreateNodeInjector function#26305
AndrewKushnir wants to merge 5 commits into
angular:masterfrom
AndrewKushnir:di-assert-rm

Conversation

@AndrewKushnir

@AndrewKushnir AndrewKushnir commented Oct 8, 2018

Copy link
Copy Markdown
Contributor

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)

Does this PR introduce a breaking change?

[ ] Yes
[x] No

@AndrewKushnir AndrewKushnir added type: bug/fix action: review The PR is still awaiting reviews from at least one requested reviewer target: major This PR is targeted for the next major release comp: ivy labels Oct 8, 2018
@AndrewKushnir
AndrewKushnir requested a review from kara October 8, 2018 19:56
@mary-poppins

mary-poppins commented Oct 8, 2018

Copy link
Copy Markdown

You can preview 9dbd148 at https://pr26305-9dbd148.ngbuilds.io/.

kara
kara reviewed Oct 8, 2018
View reviewed changes

@kara kara left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Some nits

Comment thread packages/core/test/render3/compiler_canonical/pipes_spec.ts Outdated

@kara kara Oct 8, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Slot offset here should be 1:

pipeBind(1, 1, ctx.title)

Comment thread packages/core/test/render3/compiler_canonical/pipes_spec.ts Outdated

@kara kara Oct 8, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

vars count should be 3 because internally pipeBind saves a slot for ctx.title and a slot for the result of the transform.

This doesn't happen to create a problem with this test, but we should be correct in case the implementation changes.

Comment thread packages/core/test/render3/compiler_canonical/pipes_spec.ts Outdated

@kara kara Oct 8, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: name here should be MyComponent_Factory for consistency

Comment thread packages/core/test/render3/compiler_canonical/pipes_spec.ts Outdated

@kara kara Oct 8, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: Comment should match the template with regard to pipe name

{{ title | myConcatPipe }}

@mary-poppins

mary-poppins commented Oct 8, 2018

Copy link
Copy Markdown

You can preview 2883edb at https://pr26305-2883edb.ngbuilds.io/.

@AndrewKushnir

AndrewKushnir commented Oct 8, 2018

Copy link
Copy Markdown
Contributor Author

@kara thanks for the review! I've updated the code according to the comments. Please have a look when you get a chance. Thank you.

@mary-poppins

mary-poppins commented Oct 8, 2018

Copy link
Copy Markdown

You can preview dd6d3f9 at https://pr26305-dd6d3f9.ngbuilds.io/.

@mary-poppins

mary-poppins commented Oct 9, 2018

Copy link
Copy Markdown

You can preview 74560f9 at https://pr26305-74560f9.ngbuilds.io/.

@pkozlowski-opensource pkozlowski-opensource mentioned this pull request Oct 9, 2018
Closed
kara
kara approved these changes Oct 9, 2018
View reviewed changes

@kara kara left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@kara kara removed the action: review The PR is still awaiting reviews from at least one requested reviewer label Oct 9, 2018
@AndrewKushnir AndrewKushnir added the action: merge The PR is ready for merge by the caretaker label Oct 9, 2018
@jasonaden jasonaden closed this in e564420 Oct 9, 2018
@angular-automatic-lock-bot

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

Copy link
Copy Markdown

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 Sep 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer
@kara kara kara approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

action: merge The PR is ready for merge by the caretaker cla: yes target: major This PR is targeted for the next major release type: bug/fix

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@AndrewKushnir @mary-poppins @kara @googlebot