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

Improve GetterHandler params #22

Closed

Conversation

jackkoppa
Copy link

@jackkoppa jackkoppa commented Nov 2, 2018

Hi!

Really like the package. We were recently trying to access RootState from a getter, and realized that if we follow this library's type expectations for a getter, we're not actually getting the right parameters in order to work with root state.

As mentioned in the commits, this is the line in Vuex where the type of a getter is defined: https://github.com/vuejs/vuex/blob/dev/types/index.d.ts#L97

And here it is being used in the docs: https://vuex.vuejs.org/guide/modules.html#accessing-global-assets-in-namespaced-modules

However, the current GetterHandler for this library only has 2 params (state and rootState), and by leaving out getters and rootGetters, a user trying to access rootState will try to use the 2nd argument, when in fact they should be trying to use the 3rd argument.

This PR is a potentially breaking change, because it changes the order of arguments that GetterHandler expects. However, I'd have to guess that no one up to this point would have been accessing rootState successfully, since they would have been targeting the wrong argument. So if we're OK with the change, I think the published version could still be a minor version, since it's more of a bugfix than a full API change. Totally happy to discuss. Also, we could just add a second type, GetterHandlerExtended, and read could accept either GetterHandler or GetterHandlerExtended. That way, anyone still using the original handler wouldn't see an issue. It seems like the wrong approach, though, if the current version is indeed incorrect.

Really happy to discuss, and thanks again for a great library - it's allowed us to keep using TypeScript on our project, which has been a boon

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 2c440de on jackkoppa:jackkoppa/getter-handler-improvement into 105d219 on istrib:master.

@jackkoppa
Copy link
Author

This fixes #11

@d1820
Copy link

d1820 commented Nov 2, 2018

I think solution works.. instead of new extended could also look at decorator maybe for override the Injection on existing read..

jackkoppa added a commit to politico/typesafe-vuex that referenced this pull request Nov 5, 2018
Copy of PR on vuex-typescript:
istrib/vuex-typescript#22
Fixes bug that did not allow users to access RootState from typed getters
Bug on vuex-typescript:
istrib/vuex-typescript#11
@jackkoppa
Copy link
Author

Hi all - if you come across this, please feel free to checkout https://github.com/politico/typesafe-vuex, which we did as a fork of this package. Granted, I think the likely recommendation for state management in Vue + TS apps will have changed by now, given Vue 3 architectural changes, and as I'm not currently working heavily on Vue apps, would encourage people to search for alternative state management options if starting from scratch. Closing this PR for now.

@jackkoppa jackkoppa closed this Dec 14, 2022
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.

3 participants