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): support inserting a `viewRef` that is already present #34052

Closed

Conversation

@petebacondarwin
Copy link
Member

petebacondarwin commented Nov 26, 2019

When inserting a viewRef it is possible to not provide
an index, which is regarded as appending to the end of
the container.

If the viewRef already exists in the container, then
this results in a move. But there was a fault in the logic
that computed where to insert the viewRef that did not
account for the fact that the viewRef was already in
the container, so the insertion index was outside the
bounds of the array.

Fixes #33924

Copy link
Member

pkozlowski-opensource left a comment

@petebacondarwin ha! Thnx for looking into it, I was wondering what was going on when I saw the error / issue on GH!

I left 2 comments in the PR, but when I was writing those comments I wanted to verify things and ended up doing another test + fix, see #34054...

Totally up to you to pick it up, change this PR or do whatever is right :-) In any case we want to avoid hand-written "generated" code.

pkozlowski-opensource added a commit to pkozlowski-opensource/angular that referenced this pull request Nov 26, 2019
When inserting a `viewRef` it is possible to not provide
an `index`, which is regarded as appending to the end of
the container.

If the `viewRef` already exists in the container, then
this results in a move. But there was a fault in the logic
that computed where to insert the `viewRef` that did not
account for the fact that the `viewRef` was already in
the container, so the insertion `index` was outside the
bounds of the array.

Fixes #33924
Move a view only if it would end up at a different place.
Otherwise we would do unnecessary processing like DOM manipulation, query notifications etc.

Thanks to @pkozlowski-opensource for the change.
@petebacondarwin petebacondarwin force-pushed the petebacondarwin:ivy-formly-fix branch from d57580a to c1b24d5 Nov 26, 2019
@petebacondarwin

This comment has been minimized.

Copy link
Member Author

petebacondarwin commented Nov 26, 2019

I took your perf and test suggestions. Thanks @pkozlowski-opensource.

Copy link
Member

pkozlowski-opensource left a comment

LGTM

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

AndrewKushnir commented Nov 26, 2019

@matsko matsko closed this in f0f426b Nov 26, 2019
matsko added a commit that referenced this pull request Nov 26, 2019
)

Move a view only if it would end up at a different place.
Otherwise we would do unnecessary processing like DOM manipulation, query notifications etc.

Thanks to @pkozlowski-opensource for the change.

PR Close #34052
matsko added a commit that referenced this pull request Nov 26, 2019
When inserting a `viewRef` it is possible to not provide
an `index`, which is regarded as appending to the end of
the container.

If the `viewRef` already exists in the container, then
this results in a move. But there was a fault in the logic
that computed where to insert the `viewRef` that did not
account for the fact that the `viewRef` was already in
the container, so the insertion `index` was outside the
bounds of the array.

Fixes #33924

PR Close #34052
matsko added a commit that referenced this pull request Nov 26, 2019
)

Move a view only if it would end up at a different place.
Otherwise we would do unnecessary processing like DOM manipulation, query notifications etc.

Thanks to @pkozlowski-opensource for the change.

PR Close #34052
@petebacondarwin petebacondarwin deleted the petebacondarwin:ivy-formly-fix branch Nov 27, 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.