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(hydration): should force hydrate props with .prop & .attr modifiers #7490

Closed
wants to merge 7 commits into from

Conversation

edison1105
Copy link
Member

@edison1105 edison1105 commented Jan 10, 2023

related to #7476
NOTE:
This PR does not fundamentally solve the problem; it simply aligns with the behavior of Vue 2.
see #7490 (comment)

@edison1105 edison1105 changed the title fix(hydration): should force hydrate props with .prop & ^attr modifiers fix(hydration): should force hydrate props with .prop & .attr modifiers Jan 10, 2023
@edison1105 edison1105 added the ready to merge The PR is ready to be merged. label Jul 21, 2023
@github-actions
Copy link

Size Report

Bundles

File Size Gzip Brotli
runtime-dom.global.prod.js 85.9 kB (+24 B) 32.7 kB (+8 B) 29.5 kB (+33 B)
vue.global.prod.js 132 kB (+24 B) 49.4 kB (+10 B) 44.3 kB (-49 B)

Usages

Name Size Gzip Brotli
createApp 47.9 kB 18.8 kB 17.2 kB (-2 B)
createSSRApp 50.7 kB (+24 B) 20 kB (+8 B) 18.2 kB (-1 B)
defineCustomElement 50.3 kB 19.6 kB (-1 B) 17.9 kB (+1 B)
overall 61.2 kB 23.7 kB 21.6 kB

@skirtles-code
Copy link
Contributor

I have a few questions about this change.

  1. Why do we need to force the patching of props that use .attr? Won't those be the same as what's in the SSR HTML anyway?
  2. Does this really close static non-attr bindings does not work in SSR #7476? I can see how this change would be helpful in the scenario described there, but I'm not sure whether it fully addresses the problem.
  3. As a potential follow up to this change, currently props marked with .prop are still rendered as attributes in the SSR HTML. Do you think it would be better to omit them from the HTML entirely and just set them as part of hydration?

@edison1105
Copy link
Member Author

@skirtles-code
Actually, the problem is not fundamentally solved. A better solution would be to output a js code on the SSR to set the indeterminate property. However, it seems that SSR currently lacks this capability. In Vue 2, we could modify the checkbox state during the hydration phase using :indeterminate.prop, but this doesn't work in Vue 3. This PR aims to maintain the same behavior as Vue 2.

see vuejs/vue#4094 (comment)

@yyx990803
Copy link
Member

yyx990803 commented Nov 10, 2023

#7476 really is more of a specific case for indeterminate since <input type="checkbox" indeterminate> works in CSR due to the runtime patchProps logic, so we have to special case it to make it consistent in SSR. This is done in 34b5a5d

The intention of this PR makes sense, but there are two problems:

  1. .attr modifier should not require forced hydration since it would have already been rendered by the server. The only required case is .prop.
  2. v-bind with .prop modifier is not guaranteed to enter the branch being modified here. We need to update the compiler so that elements with v-bind.prop are marked with a patch flag that makes it enter this branch.

See 364f319

With both commits all variations of indeterminate bindings should work properly in hydration: https://play.vuejs.org/#__SSR__eNqVUk1LAzEQ/StjLqsg20Nvy1ZQKagHFRW85FJ3p2tqvkgmtVL2vzub0qqgVW/JvDdv3jxmLU69L5cJRSXq2ATlCSJS8ifSKuNdIFhDwDn0MA/OQMHUQlppG2cjgYkdTAb8sLhArR08uqDbg+JI2nq0kWMh/hAar2eE/AOolfWJgN48TqRonrF5eXIrKUDZFgmDUfZXavWFyxiFhFL8p6n0wfk/dpY/jqtHu93EsaDIwcxVVy6is5zpelBlJWe80hhuPCkOTooKMjJgM87t9SrXBtHjbT1P/6a+iGyo4sdtwIhhyTZ2GM1Ch7SBp/fXuOL3DjSuTZrZe8A7jE6nweOGdpZsy7Y/8bLby3wZynYPcboitHG7VE6FmX3mS8HXcr5n9Q+743Kc+6TtRf8OSHfgNQ==

@yyx990803 yyx990803 closed this Nov 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge The PR is ready to be merged.
Projects
Status: Rejected
Development

Successfully merging this pull request may close these issues.

4 participants