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(reactivity): allowRecurse effects's dirtyLevel unreliable #10101

Merged
merged 5 commits into from
Jan 13, 2024

Conversation

johnsoncodehk
Copy link
Member

@johnsoncodehk johnsoncodehk commented Jan 13, 2024

This resolves the regression introduced by #10091 and re-fix #10082.

Also fix a test of #10085, #10090.

Copy link

github-actions bot commented Jan 13, 2024

Size Report

Bundles

File Size Gzip Brotli
runtime-dom.global.prod.js 89.8 kB (+69 B) 34.2 kB (+26 B) 30.8 kB (+16 B)
vue.global.prod.js 147 kB (+69 B) 53.5 kB (+25 B) 47.7 kB

Usages

Name Size Gzip Brotli
createApp 50.2 kB (+69 B) 19.6 kB (+16 B) 18 kB (+30 B)
createSSRApp 53.5 kB (+69 B) 20.9 kB (+16 B) 19.1 kB (-12 B)
defineCustomElement 52.5 kB (+69 B) 20.4 kB (+17 B) 18.6 kB (-2 B)
overall 63.9 kB (+69 B) 24.7 kB (+18 B) 22.4 kB (+9 B)

@johnsoncodehk
Copy link
Member Author

/ecosystem-ci run

@vue-bot
Copy link

vue-bot commented Jan 13, 2024

📝 Ran ecosystem CI: Open

suite result latest scheduled
language-tools failure failure
nuxt success success
pinia success success
quasar failure success
radix-vue success undefined undefined
router success success
test-utils success success
vant failure success
vite-plugin-vue success success
vitepress success success
vue-i18n success success
vue-macros success failure
vuetify failure success
vueuse success success
vue-simple-compiler success success

@johnsoncodehk
Copy link
Member Author

/ecosystem-ci run

@vue-bot
Copy link

vue-bot commented Jan 13, 2024

📝 Ran ecosystem CI: Open

suite result latest scheduled
language-tools failure failure
nuxt failure success
pinia success success
quasar failure success
radix-vue success undefined undefined
router success success
test-utils success success
vant failure success
vite-plugin-vue success success
vitepress success success
vue-i18n success success
vue-macros success failure
vuetify failure success
vueuse success success
vue-simple-compiler success success

@johnsoncodehk
Copy link
Member Author

/ecosystem-ci run

@vue-bot
Copy link

vue-bot commented Jan 13, 2024

📝 Ran ecosystem CI: Open

suite result latest scheduled
language-tools failure failure
nuxt failure success
pinia success success
quasar success success
radix-vue success undefined undefined
router success success
test-utils success success
vant success success
vite-plugin-vue success success
vitepress success success
vue-i18n success success
vue-macros success failure
vuetify failure success
vueuse success success
vue-simple-compiler success success

@yyx990803 yyx990803 marked this pull request as ready for review January 13, 2024 04:15
@Doctor-wu
Copy link
Contributor

CleanShot 2024-01-13 at 12 17 28@2x Just gonna to do the same thing 😂

@yyx990803 yyx990803 merged commit e45a8d2 into vuejs:main Jan 13, 2024
11 checks passed
@rocifier
Copy link

rocifier commented Mar 15, 2024

Hi, this change causes our code base to get into a deadlock situation with two components which appear to be awaiting one another's resources, though it's not obvious. Is anyone able to explain more about this fix and what behaviour it affects? We aren't using allowRecurse explicitly anywhere. We're using SFCs and Pinia stores. Pinia stores load in the background and their values are injected into the setup of components which then receive updates via computed properties that depend on storeToRefs() to map the store state to the local component state.

The way that the Pinia stores are loaded in the background is that they return their initial (previously known) state when their computed properties are accessed and as a side-effect they asynchronously fetch from apis and then update their state values. The intention behind our modified stores is that the act of setting a store state value would mark a computed property as dirty and recompute it as needed. We are depending on Vue to allow side-effects, or more specifically to regard any side-effects such as asynchronous closures as non-dirty.

As far as I can tell we have built our stores to look similar to the new test case it should work when chained(ref+computed) previously returning a not dirty flag (since we took advantage of the fact vue doesn't care about computed side-effects), whereas now with this change it returns dirty. Would the way to implement something like this now be to make a customRef which does the asynchronous wait (and lock), and can return its value but not trigger - until the async call returns at which point it sets the value again and calls trigger()

EDIT: For those reading this later, I solved the above problem by following this pattern (can be added to Computed.spec.ts):

  it('should be able to cause ignorant developer side-effects', async () => {
    let val: number = 1
    const v = customRef<number>((_track, trigger) => ({
      get: (): number => {
        // in a real-world app this would be an api call or similar
        setTimeout(() => {
            v.value = 2
            trigger()
          }, 10)
        return val
      },
      set: (value: number) => {
        val = value
        trigger()
      },
    }))

    const sideEffectWatcher = computed(() => v.value)

    expect(v.value).toBe(1)
    await new Promise((resolve) => {
      setTimeout(() => resolve(true), 15)
    })
    expect(v.value).toBe(2)
    expect(sideEffectWatcher.effect._dirtyLevel).toBe(DirtyLevels.Dirty)
  })

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.

Providing a computed prop to child breaks the reactivity of the computed
5 participants