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

feat: improve helper types for more type safety #1121

Open
wants to merge 25 commits into
base: dev
from

Conversation

Projects
None yet
@ktsn
Member

ktsn commented Jan 4, 2018

This typings update allows to use typed getters/actions/mutations out of the box if they are used in the following manner.

1. Declare each store assets types as interfaces.

// State
export interface CounterState {
  count: number
}

// Getters
// key: getter name
// value: return type of getter
export interface CounterGetters {
  power: number
}

// Mutations
// key: mutation name
// value: payload type of mutation
export interface CounterMutations {
  increment: { amount: number }
}

// Actions
// key: action name
// value: payload type of action
export interface CounterActions {
  incrementAsync: { amount: number, delay: number }
}

2. annotate a namespaced module with DefineModule utility type.

The annotated assets must fulfill specified names and return type (getters) / payload type (actions, mutations). Also the assets types will be inferred.

The type in the following example should be fully inferred:

import { DefineModule } from 'vuex'

const counter: DefineModule<CounterState, CounterGetters, CounterMutations, CounterActions> = {
  namespaced: true,

  state: {
    count: 0
  },

  getters: {
    power: state => state.count * state.count
  },

  mutations: {
    increment (state, payload) {
      state.count += payload.amount
    }
  },

  actions: {
    incrementAsync ({ commit }, payload) {
      setTimeout(() => {
        commit('increment', { amount: payload.amount })
      }, payload.delay)
    }
  }
}

3. create typed namespaced helpers with createNamespacedHelpers.

Then, we can acquire typed mapXXX helpers for the defined namespaced module.

export const counterHelpers = createNamespacedHelpers<CounterState, CounterGetters, CounterMutations, CounterActions>('counter')

4. use the namespaced helpers in a component.

import { counterHelpers } from '@/store/modules/counter'

export default Vue.extend({
  computed: counterHelpers.mapState({
    value: 'count'
  }),

  methods: counterHelpers.mapMutations({
    inc: 'increment'
  }),

  created () {
    // These are correctly typed!
    this.inc({ amount: 1 })
    console.log(this.value)
  }
})

Caveats

Store is still not typed

I think it is probably impossible to infer the entire store type correctly since we cannot concat getter/actions/mutations names with namespace on type level. So this PR focuses how we do not use $store directly but use typed helpers instead.

It does not infer the types completely if passing functions to mapXXX helpers.

For example:

counterHelpers.mapState({
  foo: state => state.count
})

counterHelpers.mapMutations({
  bar (commit) {
    commit('increment', { amount: 1 })
  }
})

We can write the above code with inferred state and commit types but the component will have a type of foo: any and bar: (...args: any[]) => any.

It can be easily rewrite with a combination of object form mapXXX helpers and normal methods, so I think it would not be a problem.

Using root assets

The default mapXXX helpers still accepts any asset names and returns record of any type. To manually annotate them, we can use createNamespacedHelpers too. If we don't specify a namespace as an argument, it returns the root mapXXX helpers so that we annotate with root module assets types as same as namespaced modules.

const rootHelpers = createNamespacedHelpers<RootState, RootGetters, RootMutations, RootActions>()

fix #532
fix #564
fix #1119

type: string;
}
export interface MutationPayload extends Payload {
type Payload<K extends keyof P, P> = { type: K } & P[K]

This comment has been minimized.

@HerringtonDarkholme

HerringtonDarkholme Jan 6, 2018

Member

Technically this is a breaking change. I don't know how frequent Payload is used, but this might be a compatible alternative:

type Payload<P = { [k: string]: {} }, K extends keyof P = keyof P> =
    { type: K } & P[K]

http://www.typescriptlang.org/play/#src=type%20Payload%3CP%20%3D%20%7B%20%5Bk%3A%20string%5D%3A%20%7B%7D%20%7D%2C%20K%20extends%20keyof%20P%20%3D%20keyof%20P%3E%20%3D%0A%20%20%20%20%7B%20type%3A%20K%20%7D%20%26%20P%5BK%5D%0A%0Adeclare%20var%20p%3A%20Payload%0A

This comment has been minimized.

@ktsn

ktsn Jan 9, 2018

Member

I ended up reverting the rename of Payload 0858c6d

This comment has been minimized.

@HerringtonDarkholme

HerringtonDarkholme Jan 9, 2018

Member

Yes, changing interface to type would be a breaking change. My mistake 😷

@HerringtonDarkholme

This comment has been minimized.

Member

HerringtonDarkholme commented Jan 6, 2018

Thanks! This is a long change so I think I need some time to review the change. Also, we need documentation update to catch up this new awesomeness. 😄

type Computed<R> = () => R;
type Method<R> = (...args: any[]) => R;
type MutationMethod<P> = (payload: P) => void;
type ActionMethod<P> = (payload: P) => Promise<any>;

This comment has been minimized.

@HerringtonDarkholme

HerringtonDarkholme Jan 6, 2018

Member

I'm fine with untyped action return type. In fact, users can register multiple actions so the return type can't be checked any way.

* mapMutations
*/
interface MapMutations<Mutations> {
<M extends Mutations = Mutations, Key extends keyof M = keyof M>(map: Key[]): { [K in Key]: MutationMethod<M[K]> };

This comment has been minimized.

@HerringtonDarkholme

HerringtonDarkholme Jan 6, 2018

Member

Usually generic default is for interface / type alias. Mapper is not exported nor augmented by users, so I think these defaults aren't required.

This comment has been minimized.

@ktsn

ktsn Jan 9, 2018

Member

Actually, it throws error if Mutations is BaseType since M will be inferred as {} in that case. I have no idea how we fix it without default generic type 😞

But we may no longer need it if we create typed root helpers with createNamespacedHelpers in any cases.

This comment has been minimized.

@HerringtonDarkholme

HerringtonDarkholme Jan 9, 2018

Member

Let's try typed root helpers! It looks more promising since it can return more precise types.

& MapperWithNamespace<Computed>
& MapperForState
& MapperForStateWithNamespace;
export declare const mapState: RootMapState<BaseType, BaseType>;

This comment has been minimized.

@HerringtonDarkholme

HerringtonDarkholme Jan 6, 2018

Member

The proposed usage is mapState<RootState>(['foo', 'bar']), however, the return type is RootState because foo/bar isn't participating inference.

Another usage is that we export RootMapXXX and slightly change their type.

export interface RootMapState<State> {
  <Keys extends keyof State>(keys: Keys[]): {[K in Keys]: State[K]}
}

then users can write something like const myMapState: RootMapState<RootState> = mapState. Of course, it requires us to maintain more public types, and also looks a little bit bizarre to end users.

But it is more versatile in usage -- I think object style can be supported, and more precise in returning type.

This comment has been minimized.

@HerringtonDarkholme

HerringtonDarkholme Jan 6, 2018

Member

How about reusing the pattern of createNameSpacedHelpers? Adding one more helper function like createRootHelpers? Implementation wise, it just return these root helpers, but it gives better developer experience for both end users and lib maintainers.

This comment has been minimized.

@ktsn

ktsn Jan 9, 2018

Member

How about reusing the pattern of createNameSpacedHelpers

That sounds good idea. I think returning root helpers from createNamespacedHelpers with no argument would work. createRootHelpers would sound a bit weird for pure JS users.

This comment has been minimized.

@HerringtonDarkholme

HerringtonDarkholme Jan 9, 2018

Member

Awesome! Great balance between types and runtime behavior.

* `ExtraGetters` is like `Getters` type but will be not defined in the infered getters object.
* `RootState` and `RootGetters` are the root module's state and getters type.
*/
export type DefineGetters<

This comment has been minimized.

@HerringtonDarkholme

HerringtonDarkholme Jan 6, 2018

Member

What about a one-for-all option? like :

export type DefineStore<
  Actions,
  State,
  Getters,
  Mutations,
  ExtraActions = {},
  RootState = {},
  RootGetters = {},
  RootMutations = {},
  RootActions = {}
> = {
  // ....
}

This comment has been minimized.

@ktsn

ktsn Jan 9, 2018

Member

Yes, it looks better since we always combine all assets for module 😄
I will rewrite the utility types.

@ktsn

This comment has been minimized.

Member

ktsn commented Jan 9, 2018

documentation update

I will write docs for this. Thanks for pointing it out!

* mapMutations
*/
interface MapMutations<Mutations> {
<M extends Mutations = Mutations, Key extends keyof M = keyof M>(map: Key[]): { [K in Key]: MutationMethod<M[K]> };

This comment has been minimized.

@Demivan

Demivan Jan 9, 2018

There is a problem with returning MutationMethod here.
MutationMethod does not have ...args: any[] parameters anymore.
This makes all mutations take one argument, same goes for actions.

This comment has been minimized.

@ktsn

ktsn Jan 9, 2018

Member

It's not actually a problem because mutations and actions only accept one parameter. The returned methods will never process 2nd or latter parameters.

This comment has been minimized.

@Demivan

Demivan Jan 9, 2018

image
This worked for me before

This comment has been minimized.

@Demivan

Demivan Jan 9, 2018

You can pass multiple parameters (or none) to mapped actions

This comment has been minimized.

@ktsn

ktsn Jan 9, 2018

Member

Ah, I see. It should be an optional argument in default to follow the existing behavior.

But I think if we annotate types via createNamespacedHelper explicitly, it should not be an optional. Because it easily breaks apps if the user adds a payload for actions/mutations with no payload and the type checker cannot detect that.

If the users want to declare non-payload actions/mutations, they should explicitly pass a null or undefined value.

@HerringtonDarkholme What do you think?

This comment has been minimized.

@HerringtonDarkholme

HerringtonDarkholme Jan 10, 2018

Member

I agree that users should explicitly pass null or undefined.

This comment has been minimized.

@blake-newman

blake-newman Jan 10, 2018

Member

I agree, i believe undefined is probably best to signal that this payload is void. null could have context to an application where undefined can't be used on the store as it breaks the reactivity principles.

<Key extends keyof Mutations>(map: Key[]): { [K in Key]: MutationMethod<Mutations[K]> };
<Map extends Record<string, keyof Mutations>>(map: Map): { [K in keyof Map]: MutationMethod<Mutations[Map[K]]> };
interface MapMutations<Mutations, Type extends MethodType> {
<Key extends keyof Mutations>(map: Key[]): { [K in Key]: MutationMethod<Mutations[K], Type> };

This comment has been minimized.

@HerringtonDarkholme

HerringtonDarkholme Jan 10, 2018

Member

This typing still requires all methods in mutation/action are homogeneous: all methods either require one parameter at the same time or don't accept parameter at all. We cannot declare such mutations that some methods require parameter while others don't at the same time.

This comment has been minimized.

@ktsn

ktsn Jan 11, 2018

Member

Yes, I intend that behavior. I leave them optional if the users do not annotate types because they probably want flexible syntax like in JS. On the other hand, the methods always require an argument if they annotate types because they probably want type safety in that case.

Create namespaced component binding helpers. The returned object contains `mapState`, `mapGetters`, `mapActions` and `mapMutations` that are bound with the given namespace. [Details](modules.md#binding-helpers-with-namespace)
If the namespace is not specified, it returns the root mapXXX helpers. This behavior is convenient to annotate strict types for mapXXX helpers.

This comment has been minimized.

@HerringtonDarkholme

HerringtonDarkholme Jan 10, 2018

Member

Hmmm, what about

This is mainly for TypeScript users to annotate root helper's type.

Mentioning TypeScript explicitly makes JS users know annotating type doesn't require much care for them.

This comment has been minimized.

@ktsn

ktsn Jan 11, 2018

Member

It sounds clearer than before. Thanks!

@@ -12,9 +12,7 @@ export {
} from "./helpers";
export {
DefineGetters,

This comment has been minimized.

@HerringtonDarkholme

HerringtonDarkholme Jan 10, 2018

Member

IMHO it doesn't bother if we re-export other utility type helpers. These helper types can be convenient for users to separate getters/actions/mutations to different files.

This comment has been minimized.

@ktsn

ktsn Jan 11, 2018

Member

Makes sense. I've exposed them again.

@blake-newman

As we are doing a large change for TS users, we should probably do this aswell: #994

So that $store can be argumented, thus giving more type safety.

@roblav96

This comment has been minimized.

roblav96 commented Mar 5, 2018

@ktsn Absolutely wonderful PR! Lobby to merge -> @yyx990803

@ktsn ktsn referenced this pull request Mar 9, 2018

Closed

Namespaced modules #2

@omidb

This comment has been minimized.

omidb commented Mar 16, 2018

Looks really nice. How does this PR compare to https://github.com/mrcrowl/vuex-typex ?

@ktsn

This comment has been minimized.

Member

ktsn commented Mar 22, 2018

@omidb vuex-typex (and probably other vuex wrapper) provide runtime helper to wrap Vuex and provide other interface to achive type safety. It is the same approach as vue-class-component. It may able to achieve complete type safety as the lib auther can modify the API into type checker frendly form but the users need to learn the wrapper API.

On the other hand, this typing update provides type-level helper and does not affect current runtime semantics in Vuex. That means the users don't need to change their code to make it type safe. They only need to annotate their state/getters/actions/mutations types with type helpers.

@blake-newman

This comment has been minimized.

Member

blake-newman commented Apr 6, 2018

@ktsn since this has been stalled, how much effort do you think this will be to add the conditional types to allow payloads to be omitted?

@roblav96

This comment has been minimized.

roblav96 commented Apr 6, 2018

@ktsn Also haven't found a way to arbitrage <any>

store?: Store<any>;

$store: Store<any>;

Current solution:
"postinstall": "rimraf node_modules/vuex/types/vue.d.ts", jk lol

@ktsn

This comment has been minimized.

Member

ktsn commented Apr 7, 2018

@blake-newman Actually, I already tried it but it is trickier than I thought at the first glance. I'll look into it deeper when I have sufficient time to do that.

@roblav96 FYI #1192

@vahdet

This comment has been minimized.

vahdet commented Apr 18, 2018

For the time, is there any way to use that import { DefineModule } from 'vuex' line? Is it available with the latest dev build?

@blake-newman

Approved, I think we should make a beta release of this, and any other small TS fix PRs. We can always fix undefined payloads after in a new release.

@HerringtonDarkholme HerringtonDarkholme referenced this pull request May 9, 2018

Closed

Better IntelliSense support for vuex #783

3 of 3 tasks complete
@lzgrzebski

This comment has been minimized.

lzgrzebski commented May 14, 2018

any ETA when this will be released? Currently I have to patch my ts code with some explicit 'any' to make it work which isn't ideal.. :<

@uoc1691

This comment has been minimized.

uoc1691 commented Jun 1, 2018

Great Work! Question on the usage.
Can the helper be used like
storeHelper.mapMutations(["doSomething"]).doSomething({event: new MyCustomEvent()});

Without doing

methods: counterHelpers.mapMutations({ inc: 'increment' })

@kirilvit

This comment has been minimized.

kirilvit commented Jun 27, 2018

Please, resolve the conflicts

@uoc1691

This comment has been minimized.

uoc1691 commented Jun 28, 2018

Within an action "this" refers to the store instance. Can we modify DefineActions type like below?

type DefineActions<
    Actions,
    State,
    Getters,
    Mutations,
    ExtraActions = {},
    RootState = {},
    RootGetters = {},
    RootMutations = {},
    RootActions = {}
    > = {
      [K in keyof Actions]: (this: Store<RootState>,
        ctx: StrictActionContext<State, RootState, Getters, RootGetters, Mutations, RootMutations, Actions & ExtraActions, RootActions>,
        payload: Actions[K]
      ) => Promise<any> | void
    }
@ffxsam

This comment has been minimized.

ffxsam commented Jul 25, 2018

What's holding up this PR from being merged?

Can anyone tell me what a good workaround is, in the mean time? I'm not that fluent in TypeScript.

@hobotroid

This comment has been minimized.

hobotroid commented Aug 15, 2018

I too would love this PR to get merged.

@RehanSaeed

This comment has been minimized.

RehanSaeed commented Aug 31, 2018

Looks like @yyx990803 is yet to review this PR.

@zhangbobell

This comment has been minimized.

zhangbobell commented Sep 2, 2018

It will make strong sense for this PR been merged.

@latel

This comment has been minimized.

latel commented Sep 3, 2018

strill waiting for this pr

@Raiondesu

This comment has been minimized.

Raiondesu commented Sep 24, 2018

Every maintainer of this repo seems to have forgotten about all type-improving pull requests...

@ffxsam

This comment has been minimized.

ffxsam commented Oct 22, 2018

@ktsn Any updates on this? I wrote a Vue + TS cookbook, and provide a workaround for this problem:

https://github.com/ffxsam/vue-typescript-cookbook#im-using-vuex-mapstate-or-mapgetters-and-typescript-is-saying-the-mapped-stategetters-dont-exist-on-this

But it would be nice if it worked properly out of the box.

@ffxsam

This comment has been minimized.

ffxsam commented Nov 13, 2018

Could someone please merge this? @yyx990803 @blake-newman @HerringtonDarkholme

@kirilvit

This comment has been minimized.

kirilvit commented Nov 14, 2018

Could someone please merge this? @yyx990803 @blake-newman @HerringtonDarkholme

I think this PR will not be ever merged, because TypeScript support with breaking changes is announced to be in VueJs 3 https://medium.com/the-vue-point/plans-for-the-next-iteration-of-vue-js-777ffea6fabf

@ffxsam

This comment has been minimized.

ffxsam commented Nov 14, 2018

@kirilvit That's a great point, I totally didn't think of that.

@VSDekar

This comment has been minimized.

VSDekar commented Nov 16, 2018

But at least a statement would be fair, instead of just saying nothing...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment