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(runtime-dom): before update should also set css vars #11561

Merged
merged 10 commits into from
Nov 14, 2024

Conversation

linzhe141
Copy link
Contributor

@linzhe141 linzhe141 commented Aug 8, 2024

before update should also set css vars

this pr playground case1

this pr playground case2

Copy link

github-actions bot commented Aug 8, 2024

Size Report

Bundles

File Size Gzip Brotli
runtime-dom.global.prod.js 100 kB (+16 B) 38 kB (+6 B) 34.2 kB (+11 B)
vue.global.prod.js 159 kB (+16 B) 58 kB (+8 B) 51.5 kB (+6 B)

Usages

Name Size Gzip Brotli
createApp (CAPI only) 47 kB 18.3 kB 16.7 kB
createApp 55.1 kB 21.3 kB 19.5 kB
createSSRApp 59.1 kB 23 kB 20.9 kB
defineCustomElement 59.9 kB 22.9 kB 20.8 kB
overall 68.8 kB 26.4 kB 23.9 kB

@linzhe141 linzhe141 changed the title fix(runtime-dom): apply css vars before mount and before update fix(runtime-dom): before update should also set css vars Aug 8, 2024
@@ -47,6 +48,10 @@ export function useCssVars(getter: (ctx: any) => Record<string, string>) {
watchPostEffect(setVars)
})

onBeforeUpdate(() => {
watchPostEffect(setVars)
Copy link
Member

Choose a reason for hiding this comment

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

Should just call setVars and not create another watcher on each update.

Copy link
Contributor Author

@linzhe141 linzhe141 Aug 9, 2024

Choose a reason for hiding this comment

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

When I just call setVars before update, the vnode is still a v-if comment, and it doesn't make sense to set custom properties for this comment.

image

But when I combine it with watchPostEffect, the vnode is correct.

image

Copy link
Member

Choose a reason for hiding this comment

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

or use queuePostFlushCb instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks

@edison1105
Copy link
Member

Maybe we should add corresponding test cases to prevent regression.
Could you please try to add one?

@linzhe141
Copy link
Contributor Author

Maybe we should add corresponding test cases to prevent regression. Could you please try to add one?

Of course, I'll add the necessary test cases to prevent any regression.

@linzhe141
Copy link
Contributor Author

Maybe we should add corresponding test cases to prevent regression. Could you please try to add one?

If I want to reproduce this issue through single test, it seems that I can only do it through e2e test, and simple unit test does not seem to be able to reproduce it.

@edison1105
Copy link
Member

@linzhe141
only need to check if css vars is set in onMounted

// useCssVars.spec.ts
test('with delay mount child', async () => {
    const state = reactive({ color: 'red' })
    const value = ref(false)
    const root = document.createElement('div')

    const Child = {
      setup(){
        onMounted(()=>{
          const childEl = root.children[0]
          expect(getComputedStyle(childEl!).getPropertyValue(`--color`)).toBe(`red`)
        })
        return () => h('div',{id:'childId'})
      }
    }
    const App = {
      setup() {
        useCssVars(() => state)
        return () => value.value ? h(Child) :[h('span')]
      },
    }

    render(h(App), root)
    await nextTick()
    // css vars use with fallback tree
    for (const c of [].slice.call(root.children as any)) {
      expect((c as HTMLElement).style.getPropertyValue(`--color`)).toBe(`red`)
    }

    // mount child
    value.value = true
    await nextTick()
    for (const c of [].slice.call(root.children as any)) {
      expect((c as HTMLElement).style.getPropertyValue(`--color`)).toBe(`red`)
    }
  })

@linzhe141
Copy link
Contributor Author

@linzhe141 only need to check if css vars is set in onMounted

// useCssVars.spec.ts
test('with delay mount child', async () => {
    const state = reactive({ color: 'red' })
    const value = ref(false)
    const root = document.createElement('div')

    const Child = {
      setup(){
        onMounted(()=>{
          const childEl = root.children[0]
          expect(getComputedStyle(childEl!).getPropertyValue(`--color`)).toBe(`red`)
        })
        return () => h('div',{id:'childId'})
      }
    }
    const App = {
      setup() {
        useCssVars(() => state)
        return () => value.value ? h(Child) :[h('span')]
      },
    }

    render(h(App), root)
    await nextTick()
    // css vars use with fallback tree
    for (const c of [].slice.call(root.children as any)) {
      expect((c as HTMLElement).style.getPropertyValue(`--color`)).toBe(`red`)
    }

    // mount child
    value.value = true
    await nextTick()
    for (const c of [].slice.call(root.children as any)) {
      expect((c as HTMLElement).style.getPropertyValue(`--color`)).toBe(`red`)
    }
  })

thanks!!!!!!

@edison1105 edison1105 added 🔨 p3-minor-bug Priority 3: this fixes a bug, but is an edge case that only affects very specific usage. scope: sfc-style-vars ready for review This PR requires more reviews labels Aug 9, 2024
Copy link

pkg-pr-new bot commented Oct 30, 2024

Open in Stackblitz

@vue/compiler-core

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

@vue/compiler-dom

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

@vue/compiler-ssr

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

@vue/compiler-sfc

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

@vue/reactivity

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

@vue/runtime-core

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

@vue/runtime-dom

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

@vue/server-renderer

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

@vue/shared

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

vue

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

@vue/compat

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

commit: 8e48b10

@yyx990803
Copy link
Member

yyx990803 commented Nov 14, 2024

This actually seems to be fixing a difference case from #11533
In #11533 the styles are applied by the child on mount, whereas this PR fixes the case where the styles are applied by the parent on update.

#11533 is fixed by 2d5c5e2, but this PR is also a legit fix for the different test case it demonstrated.

@yyx990803 yyx990803 merged commit c4312f9 into vuejs:main Nov 14, 2024
13 checks passed
@linzhe141 linzhe141 deleted the fix-useCssVars branch November 14, 2024 09:03
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 for review This PR requires more reviews scope: sfc-style-vars
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants