vuex-typescript
vuex-typescript copied to clipboard
istrib
Improve GetterHandler params
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
Coverage remained the same at 100.0% when pulling 2c440de9e87dac31d7ec0486bf68a99662ab3d03 on jackkoppa:jackkoppa/getter-handler-improvement into 105d21977d0f870910d86f7706c06ee8bd3bded9 on istrib:master.
I think solution works.. instead of new extended could also look at decorator maybe for override the Injection on existing read..
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.