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: keep computed getter be reactive after registering new modules #2201

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

Conversation

Azurewarth0920
Copy link

@Azurewarth0920 Azurewarth0920 commented Dec 9, 2022

close: #2197

The computed getters will be unreactive after registering modules,

Approach:

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

@netlify
Copy link

netlify bot commented Dec 9, 2022

Deploy Preview for vuex-docs ready!

Name Link
🔨 Latest commit a59d5cd
🔍 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.

@@ -925,4 +925,31 @@ describe('Modules', () => {
/getters should be function but "getters\.test" in module "foo\.bar" is true/
)
})

it('module: computed getter should be reactive after module registration', () => {
Copy link
Author

@Azurewarth0920 Azurewarth0920 Dec 9, 2022

Choose a reason for hiding this comment

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

@Azurewarth0920
Copy link
Author

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
Copy link

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

@Azurewarth0920
Copy link
Author

Azurewarth0920 commented Dec 15, 2022

@1085383233

#2201 (comment)

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

@Azurewarth0920
Copy link
Author

@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
Copy link

@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
Copy link

Any news on this?

The bug is really a bummer.

@Azurewarth0920
Copy link
Author

@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
Copy link

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

@alecgibson
Copy link

@kiaking any word on this?

alecgibson added a commit to reedsy/vuex that referenced this pull request Aug 31, 2023
Fixes vuejs#2197

At the moment, when registering a dynamic module, we call
`resetStoreState()` just to register the getters for the new module.

It seems unnecessary to reset the entire store state in this case, and
this actually also leads to [other issues][1].

This change is based on the test case added in
vuejs#2201

The approach taken in this change is to refactor the getter registration
into its own function, and call that new method when registering a
dynamic module instead of resetting the store state.

[1]: vuejs#2197
@alecgibson
Copy link

alecgibson commented Aug 31, 2023

I've raised an alternative solution in #2234

alecgibson added a commit to reedsy/vuex that referenced this pull request Aug 31, 2023
Fixes vuejs#2197

At the moment, when registering a dynamic module, we call
`resetStoreState()` just to register the getters for the new module.

It seems unnecessary to reset the entire store state in this case, and
this actually also leads to [other issues][1].

This change is based on the test case added in
vuejs#2201

The approach taken in this change is to refactor the getter registration
into its own function, and call that new method when registering a
dynamic module instead of resetting the store state.

[1]: vuejs#2197
alecgibson added a commit to reedsy/vuex that referenced this pull request Sep 1, 2023
Fixes vuejs#2197

At the moment, when registering a dynamic module, we call
`resetStoreState()` just to register the getters for the new module.

It seems unnecessary to reset the entire store state in this case, and
this actually also leads to [other issues][1].

This change is based on the test case added in
vuejs#2201

The approach taken in this change is to refactor the getter registration
into its own function, and call that new method when registering a
dynamic module instead of resetting the store state.

[1]: vuejs#2197
@alecgibson
Copy link

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 👏🏼 )

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.

Getters - Reactivity broken after registering new module
5 participants