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 #5592: comment vnode should not be merged into text vnode. #5593

Merged
merged 3 commits into from May 7, 2017

Conversation

maggiehe
Copy link
Contributor

@maggiehe maggiehe commented May 3, 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

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

Other information:

@blake-newman
Copy link
Member

This fixes one issue with vnode issues. svg is not correctly generating vnode which causes further issues with SSR. I will further investigate this, there will be some changes i will recommend but will do once i have discovered the further issues.

@blake-newman
Copy link
Member

Upon further investigation i have narrowed my issue down to my SVG component being an asynchronous component.

In the docs:

This makes it a bit tricky to use async components at arbitrary locations in your app (we will likely improve this in the future). However, it works seamlessly if you do it at the route level.

I think this PR is fine other than probably needing an isFalse helper.

@maggiehe
Copy link
Contributor Author

maggiehe commented May 4, 2017

@blake-newman Agree, I have added it beside the isTrue method.

@blake-newman
Copy link
Member

I'll ping @yyx990803 to see if he can review to ensure that everything is covered and there are no other possible side affects.

is this crucial for this to be fixed ASAP for you?

@maggiehe
Copy link
Contributor Author

maggiehe commented May 4, 2017

It is not very urgent, I have been temporarily downgraded the version to 2.2.6.
Thanks for your help:)

@yyx990803 yyx990803 merged commit a8da4fb into vuejs:dev May 7, 2017
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.

None yet

3 participants