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 #6687 out-in transition getting stuck with v-if #7023

Merged
merged 4 commits into from Nov 16, 2017
Merged

fix #6687 out-in transition getting stuck with v-if #7023

merged 4 commits into from Nov 16, 2017

Conversation

neelance
Copy link
Contributor

@neelance neelance commented Nov 8, 2017

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

An out-in transition could get stuck if the child with v-if was not yet shown and then replaced with a different child. In this case, "afterLeave" would never get fired, thus "_leaving" stayed true indefinitely. This was likely to happen with vue-router and asynchronous data loading before the new page is shown.

Fixes #6687.

An out-in transition could get stuck if the child with v-if was not yet shown and then replaced with a different child. In this case, "afterLeave" would never get fired, thus "_leaving" stayed true indefinitely. This was likely to happen with vue-router and asynchronous data loading before the new page is shown.

Fixes #6687.
@neelance
Copy link
Contributor Author

neelance commented Nov 8, 2017

Flow is unhappy with Node.COMMENT_NODE. I don't have that much experience with flow. What's the best way to fix this?

@dsonet
Copy link
Contributor

dsonet commented Nov 9, 2017

I guess just use the constant value 8 would be fine.

@neelance
Copy link
Contributor Author

neelance commented Nov 9, 2017

I've changed it to the constant value. One of the tests is still failing. I don't know enough about this code to say if this failure is really something bad or if the behavior is still fine this way and the test could be changed. Can someone with more knowledge on this please help me here?

@dsonet
Copy link
Contributor

dsonet commented Nov 9, 2017

Seems you have to update the test to reflect your fix.
The failed test is 'async components with transition-mode out-in'.
But also maybe your fix introduced new bug.

@@ -160,6 +160,7 @@ export default {
if (
oldChild &&
oldChild.data &&
oldChild.elm.nodeType !== 8 && // Node.COMMENT_NODE
Copy link
Member

@jkzing jkzing Nov 10, 2017

Choose a reason for hiding this comment

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

oldChild.elm could be undefined, so checking existence would be necessary.

oldChild.elm && oldChild.elm.nodeType !== 8

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In which situation would this happen?

Copy link
Member

Choose a reason for hiding this comment

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

This is why 'async components with transition-mode out-in' test failing. 🙂

@neelance
Copy link
Contributor Author

@jkzing You are right, this indeed fixed the test. Thanks!

I'm a bit surprised, because the log output of the test failure didn't show any error related to undefined.

Is this good to merge now?

@neelance
Copy link
Contributor Author

Any update?

@yyx990803
Copy link
Member

Thanks for the PR @neelance ! I tweaked the implementation a little bit because the transition component is reused in Weex and should ideally be decoupled from the DOM.

@yyx990803 yyx990803 merged commit 45d7ba8 into vuejs:dev Nov 16, 2017
@neelance
Copy link
Contributor Author

@yyx990803 Cool! Thanks for merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Transitioning between components fails when component's root node has v-if="false"
4 participants