-
-
Notifications
You must be signed in to change notification settings - Fork 8.4k
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(ssr): apply ssr props to the the fallback vnode-based branch in ssr #7247
Conversation
❌ Deploy Preview for vuejs-coverage failed.
|
@@ -1,5 +1,5 @@ | |||
import { createApp, createVNode } from 'vue' | |||
import { renderToString } from '../src/renderToString' | |||
import { renderToString } from '../src' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this change ensure initDirectivesForSSR()
is called in test.
Size ReportBundles
Usages
|
/ecosystem-ci run |
📝 Ran ecosystem CI: Open
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change looks good to me.
A couple of observations:
- The test is based on the original issue, which makes it look a bit more complicated than it needs to be. The slot and recursion aren't really necessary to hit the problem. Just a parent template like
<component is="div"><one v-show="false" /></component>
is enough, with a child of<div />
. It took me a while to understand what was going on because of the extra complexity in the test case. - The code is modifying the
vnode
, withvnode.props =
. I'm not sure whether this is potentially a problem. I couldn't find anywhere else in this section of the code that modifies the original VNode like this, though that may just be a coincidence. Maybe there could be a problem if the VNode is reused somehow, or if some other code is expecting to access the unaltered props? Not sure what that would look like in practice, so it may be a non-issue.
close #6123