vuex

vuex copied to clipboard

DISCLAIMER: The use of conditional types and ReturnType<T> in this PR requires the user to use at least Typescript 2.8. Right now the use of generic parameter defaults in helper.d.ts only requires Typescript 2.3.

This PR improves the typings of Store. Right now, given code like

let moduleC = {
    state() {
        return {
            valueC: -13
        }
    }
}
let moduleA = {
    state() {
        return ({
            valueA: "6",
        })
    },
    modules: {
        c: moduleC,
    }
}
let moduleB = {
    state: {
        a: 36,
    }
}
let store = new Vuex.Store({
    state: () => ({
        foo: 6
    }),
    modules: {
        a: moduleA,
        b: moduleB,
    }
})

would result in the type of store.state to only consider the local state in the root. Also store.state would be typed as a function and not as its return type. With this PR the type of store.state will correspond to

{
    foo: number;
    a: {
        valueA: string;
        c: {
            valueC: number;
        }
    };
    b: {
        a: number;
    };
}

Compatibility

We provided defaults for new generics, therefore one should be able to still use the types as before.

About older TS versions

There seems to be a strange bug in older versions of Typescript. Sometimes the type of a store differs depending on wheather you use the { state() { return {...}}, ... } or the { state: () => ({...}), ... } syntax. Its not that much of a problem though. We tested the typings in 2.8.4 and the only thing that happens was that the properties are missing, but TS does not throw an error (as long as you don't try to access one of these properties) In newer versions of Typescript everything is fine.

Thank you so much for the PR! Yeah this kinda makes sense. It doesn't address the issue of dynamic module registration but seems like it's better than the current where we have to specify whole module tree interface manually.

@ktsn what do you think about this? If we don't find any risk, maybe it's nice to have?

BTW sorry for messing up the commit names. I can create a new PR with proper commit names and tests if desired.

Concerning dynamic module registration: I don't think it is possible to do it all automatically there, except maybe store.registerModule would return the store instance (this way we we could return the store with updated typings).

Commit massage should be fine, we can squash and rename it when merging the PR 👍 Though the test code should be added before merging.

But, please let's wait until we get response from Katashin, I would like to make sure this is the right direction we wanna take 🙇

@ktsn Hi! Just checking in here 😳 Any thought on this issue?

It looks fine to me.

Thanks for your follow up! 🙂

I will apply the state typing to method parameters (where possible) and will update the tests ASAP

The typings are used by more methods and tests for the new typings are added.

I think this PR is ready to merge, but please feel free to tell me if there is anything I should alter 🙂

When do proposals like this get evaluated for merging? The functionality described here seems pretty important.