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): resolveCssVars filters non-string and number value #12442

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

yangxiuxiu1115
Copy link
Contributor

fix #12439

Copy link

pkg-pr-new bot commented Nov 20, 2024

Open in Stackblitz

@vue/compiler-core

pnpm add https://pkg.pr.new/@vue/compiler-core@12442

@vue/compiler-dom

pnpm add https://pkg.pr.new/@vue/compiler-dom@12442

@vue/compiler-ssr

pnpm add https://pkg.pr.new/@vue/compiler-ssr@12442

@vue/compiler-sfc

pnpm add https://pkg.pr.new/@vue/compiler-sfc@12442

@vue/reactivity

pnpm add https://pkg.pr.new/@vue/reactivity@12442

@vue/runtime-core

pnpm add https://pkg.pr.new/@vue/runtime-core@12442

@vue/runtime-dom

pnpm add https://pkg.pr.new/@vue/runtime-dom@12442

@vue/server-renderer

pnpm add https://pkg.pr.new/@vue/server-renderer@12442

@vue/shared

pnpm add https://pkg.pr.new/@vue/shared@12442

vue

pnpm add https://pkg.pr.new/vue@12442

@vue/compat

pnpm add https://pkg.pr.new/@vue/compat@12442

commit: 86de253

Copy link

github-actions bot commented Nov 20, 2024

Size Report

Bundles

File Size Gzip Brotli
runtime-dom.global.prod.js 100 kB (+133 B) 38 kB (+38 B) 34.2 kB (+38 B)
vue.global.prod.js 158 kB (+133 B) 57.8 kB (+41 B) 51.5 kB (+84 B)

Usages

Name Size Gzip Brotli
createApp (CAPI only) 47.2 kB 18.3 kB 16.8 kB
createApp 55.2 kB 21.3 kB 19.5 kB
createSSRApp 59.3 kB 23.1 kB 21 kB
defineCustomElement 60.1 kB 22.9 kB 20.8 kB
overall 69.1 kB 26.5 kB 24.1 kB

@yangxiuxiu1115 yangxiuxiu1115 changed the title fix: . fix(hydration): resolveCssVars filters non-string and number value Nov 20, 2024

Choose a reason for hiding this comment

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

Copilot reviewed 2 out of 2 changed files in this pull request and generated no suggestions.

Comments skipped due to low confidence (1)

packages/runtime-core/tests/hydration.spec.ts:2213

  • The variable name 'backGroundColor' is inconsistent with naming conventions. It should be renamed to 'backgroundColor'.
const backGroundColor = ref<any>(null)
String(cssVars[key]),
)
const value = cssVars[key]
if (isString(value) || typeof value === 'number') {
Copy link
Member

Choose a reason for hiding this comment

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

should also take into account an empty string
see Playground with this PR

Copy link
Member

Choose a reason for hiding this comment

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

Do we even need to take care of values apart from null and undefined here? Even a string can cause mismatch here. eg. ";"

@edison1105 edison1105 added 🔨 p3-minor-bug Priority 3: this fixes a bug, but is an edge case that only affects very specific usage. scope:hydration wait changes labels Nov 20, 2024
@edison1105
Copy link
Member

The isRenderableAttrValue should be applied here to filter values, aligning with SSR behavior.

function setVarsOnNode(el: Node, vars: Record<string, string>) {

style.setProperty(`--${key}`, vars[key])
cssText += `--${key}: ${vars[key]};`
const value = vars[key]
if (isRenderableAttrValue(value)) {
Copy link
Member

Choose a reason for hiding this comment

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

A potential breaking change might have been introduced here. Previously, the client would render undefined value, effectively setting the rule value to unset. However, now undefined value will not be rendered, causing the rule to inherit value from the parent node. However, this is an edge case.
Notably, this change aligns with SSR behavior.

Copy link

@adamdehaven adamdehaven Nov 20, 2024

Choose a reason for hiding this comment

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

Note: I don’t think rendering undefined was proper to render regardless; it was essentially setting the value to a string of ’undefined’ which simply doesn’t map to a value.

This worked because undefined is never a valid value in CSS.

In these scenarios where we want to unset and prevent inheritance, it should actually be set to a value of initial instead, which unsets the value in the current scope.

I was thinking you could use @property syntax to declare the custom property and control inheritance; however, this cannot be defined inline on an element.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔨 p3-minor-bug Priority 3: this fixes a bug, but is an edge case that only affects very specific usage. ready to merge The PR is ready to be merged. scope:hydration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SSR hydration errors when combining inline styles with v-bind() in CSS
4 participants