vuejs / vuex

🗃️ Centralized State Management for Vue.js.
https://vuex.vuejs.org
MIT License
28.42k stars 9.58k forks source link

fix: keep computed getter be reactive after registering new modules #2201

Open Azurewarth0920 opened 1 year ago

Azurewarth0920 commented 1 year ago

close: #2197

The computed getters will be unreactive after registering modules,

Approach:

When stopping effectScope, only stop the effect that has no dependencies.

netlify[bot] commented 1 year ago

Deploy Preview for vuex-docs ready!

Name Link
Latest commit a59d5cd57e9b1d03b0d07e822edd8baef4cda572
Latest deploy log https://app.netlify.com/sites/vuex-docs/deploys/639f187915d2550009d98d30
Deploy Preview https://deploy-preview-2201--vuex-docs.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Azurewarth0920 commented 1 year ago

Hi! @kiaking @posva

I think this PR may fix the broken feature in v4.1.0. if you have time, can you review this PR?

melody001du commented 1 year ago

@Azurewarth0920 Hi! There may be a problem with the unregisterModule function.

Azurewarth0920 commented 1 year ago

@1085383233

https://github.com/vuejs/vuex/pull/2201#issuecomment-1350325599

Hi! Can you provide a test case about the problem?

Azurewarth0920 commented 1 year ago

@1085383233

Hi! There may be a problem with the unregisterModule function.

Is the ReactiveEffect with the module kept alive even if the module is unregistered?

melody001du commented 1 year ago

@1085383233

Hi! There may be a problem with the unregisterModule function.

Is the ReactiveEffect with the module kept alive even if the module is unregistered?

@Azurewarth0920 Yeah, but I didn't test it.

catalin-bratu commented 1 year ago

Any news on this?

The bug is really a bummer.

Azurewarth0920 commented 1 year ago

@catalin-bratu This PR is waiting for reviews.

At this time, the workaround for my project is that downgrade the vuex to v4.0.2.

enkil2003 commented 1 year ago

@Azurewarth0920 is there any known bug for this fix you want to merge? any updates on this?

alecgibson commented 1 year ago

@kiaking any word on this?

alecgibson commented 1 year ago

~I've raised an alternative solution in https://github.com/vuejs/vuex/pull/2234~

alecgibson commented 1 year ago

I've closed my alternative solution, which fails this test case:

  it('module: computed getter should be reactive after module unregistration', () => {
    const store = new Vuex.Store({
      state: {
        foo: 0
      },
      getters: {
        getFoo: state => state.foo
      },
      mutations: {
        incrementFoo: state => state.foo++
      }
    })

    const computedFoo = computed(() => store.getters.getFoo)
    store.commit('incrementFoo')
    expect(computedFoo.value).toBe(1)

    store.registerModule('bar', {
      state: {
        bar: 0
      }
    })
    store.unregisterModule('bar')

    store.commit('incrementFoo')
    expect(computedFoo.value).toBe(2)
  })

@Azurewarth0920 do you think it's worth adding this test case to your PR? (Your code passes it 👏🏼 )