Dynalon / reactive-state

Redux-clone build with strict typing and RxJS down to its core. Wrist-friendly, no boilerplate or endless switch statements
MIT License
138 stars 7 forks source link

Fix type inference of createSlice #11

Closed martaver closed 6 years ago

martaver commented 6 years ago

Use a lookup type to infer the type of the keyed property in createSlice. Currently, type inference for creating a new slice is broken - if you create a slice on a valid property, it will always return {} or the type of your initialState.

I think this was missed because in your unit tests test_slicing.ts you declare the type of the sliced store explicitly, so that you can re-use variables between your tests with beforeEach. If you set up a test completely local then the bug is visible.

coveralls commented 6 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling bb30b03e70f6a21b2b2535e7944626f9ade5b080 on Martaver:patch-1 into 3794e24ecd769521e9a5890517fb06c0afa1c609 on Dynalon:master.

Dynalon commented 6 years ago

Yes, I was aware of the limitation (thats why all examples on .createSlice() explicitly specify a type argument) and would love to get this fixed. However, when compiling your PR locally get the same errors as the Travis CI for your PR: https://travis-ci.org/Dynalon/reactive-state/jobs/331476200#L1386

What version of Typescript did you use? I tried upgrading to TS 2.6, but with the same errors. Missing a change in the commit?

Dynalon commented 6 years ago

After fixing the type of the cleanupState argument to S[K] (line 145) and changing the type of the left-hand side of the assignmend in line 153, the store.ts code compiled AND automatic type inference works. Thank you so much for this!

The only errors left are now the example and tests - places where the type argument was given that now fail. As far as I can tell it is because K is the type argument but the (correct) inferred type is S[K] when giving the string of the property to create the slice from. If you have any idea how to fix this, all input welcome :)

The reason is at it will break existing code, so this change must go into a next major version (and it will). I'd rather have compatible way so I can commit it right away.

martaver commented 6 years ago

Going through these failed tests and in many cases simply removing the type arg fixed the issue. In the other cases, the type argument is being used to assert the type of the slice, for example where it used to be SliceState | undefined it's asserted as SliceState - this is basically a type cast and could be a source of errors. In general we want to be able to explicitly handle optional types.

To shut up those cases I just used casts. But in general I think the tests should be changed to gracefully handle the optional state property.

Also seems some of the problematic tests in test_initial_state.ts was cleared up, e.g. "should not allow non-plain objects for the slice store as cleanupState". Now the TS compiler is able to prevent erroneous values.

Dynalon commented 6 years ago

GitHub is giving me a headache, have reverte the merge to master (with force git push) and merged your commit (plus the required fixes for the tests) into the development branch.

After some more testing with my satellite projects I will release a new version with the changes (which will be v1.x since this breaks the previous API).

Dynalon commented 6 years ago

Thanks for you help! I was not away of the kind of K extends keyof S and referring to it as S[K] construct in TS :)

Dynalon commented 6 years ago

FYI: If published the development branch as alpha version to npm, you can install it in your project with npm install reactive-state@next