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

fix replaceReducer with a store enhancer #3524

Merged
merged 21 commits into from
Sep 3, 2019

Conversation

cellog
Copy link
Contributor

@cellog cellog commented Aug 24, 2019

This fixes #3482 (replaceReducer typing fails for store enhancers that update the state or extend the store) without requiring any change to the use of existing types (fully typing backwards compatible).

It adds 2 new templates to the generic for defining a Store, the extension to state, and the extension to the store definition.

By adding these 2 template parameters, we can mix them with the new reducer state from replaceReducer:

  replaceReducer<NewState = S, NewActions extends A = A>(
    nextReducer: Reducer<NewState, NewActions>
  ): Store<NewState & StateExt, NewActions, StateExt, Ext> & Ext

and return a store that has the correct typings for both state and the store extension.

To enable this functionality for store enhancers, it requires these 3 related changes:

  1. the definition for createStore() needs to be augmented to pass these values to the return type Store. Since this is an overloaded function, the StoreCreator interface that defines createStore is updated for both of the overloads. See the note below about the default value for StateExt and Ext.
  2. the StoreEnhancer type needs to re-define the first createStore parameter passed to it. It now uses, and thus requires the StateExt and Ext, otherwise it expects the returned store not to have these properties. The point of a store enhancer is to return the enhanced store from createStore. The only reason this didn't fail in the past was because we didn't pass the store enhancer extension values to createStore.
  3. The returned Store from StoreEnhancer needs to have the StateExt and Ext extensions passed to it to enable the fix in replaceReducer.

One other crucial note:

By default, the StateExt is defined to be the initial state. Originally, the PR had {} as the default value for StateExt, but this will fail if the state is defined as an array, or any other non-object value. By using S, this allows the default case to work for all possible values of the state shape. Ext is defined as {} because the store definition mixed with {} cancels out the {}.

I also added a test for store enhancer with replaceReducer, to verify correctness of the new typing, and a test for a non-object store value to the store test.

UPDATE:
It turns out using StateExt = S causes replaceReducer to fail. Instead, I changed it to check the base type. If it extends an object, {} will be a no-op. Otherwise, we return the type S, which will be one of the primitive types. There is no easy way to guarantee the type won't interfere with replaceReducer, but the assumption I am making is that if someone's root reducer isn't returning an object, it probably isn't going to call replaceReducer either.

@cellog
Copy link
Contributor Author

cellog commented Sep 3, 2019

@timdorr can we merge this prior to #3561?

index.d.ts Outdated
* assumption is that root reducers that only return a basic type (string, number, null, symbol) probably won't
* be using replaceReducer anyways.
*/
export type BaseType<S> = S extends {} ? {} : S
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current name isn't descriptive. This should be called EnsureObject.

@timdorr
Copy link
Member

timdorr commented Sep 3, 2019

I don't agree with the approach here. It is totally valid to return a non-object from the reducer and to replace it with another reducer that does the same. We need to support that case, otherwise this is incorrectly typed.

@cellog
Copy link
Contributor Author

cellog commented Sep 3, 2019

I don't agree with the approach here. It is totally valid to return a non-object from the reducer and to replace it with another reducer that does the same. We need to support that case, otherwise this is incorrectly typed.

The basic approach of the PR is to explicitly pass in state and store extension to the definition of Store so that we can access them in replaceReducer. Is that what you disagree with? Or just the implementation detail of the limitation on what state extension can be? If it is the limitation on State extension, the obvious solution is to remove the extends clause altogether, and also BaseType. No need to scrap the whole PR for that :)

@timdorr
Copy link
Member

timdorr commented Sep 3, 2019

Yeah, my problem isn't with the state/store extensions, just the the objectification of the StateExt.

@cellog
Copy link
Contributor Author

cellog commented Sep 3, 2019

Yeah, my problem isn't with the state/store extensions, just the the objectification of the StateExt.

ok, let me see what I can wrangle

index.d.ts Show resolved Hide resolved
@@ -51,7 +51,7 @@ export default function applyMiddleware<Ext, S = any>(
): StoreEnhancer<{ dispatch: Ext }>
export default function applyMiddleware(
...middlewares: Middleware[]
): StoreEnhancer {
): StoreEnhancer<any> {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this fix ends up being fine because the overloads provide more restrictive typing (you can check in tests/typescript/middleware.ts to see that the stores returned are strictly typed)

@cellog
Copy link
Contributor Author

cellog commented Sep 3, 2019

I don't agree with the approach here. It is totally valid to return a non-object from the reducer and to replace it with another reducer that does the same. We need to support that case, otherwise this is incorrectly typed.

OK, here is what I came up with.

Instead of trying to get a base type that we can always extend the state with, the state is only extended when there is an extension present. I did this by changing the default value of StateExt to never, and then adding a check to see if it is exactly never with:

export type ExtendState<State, Extension> = [Extension] extends [never]
  ? State
  : State & Extension

This works because a tuple with never as its only member only matches a tuple with never. So, if the user passes any extension to state, it will fail the test, and in that case we return the intersection of the state and its extension. If that fails, it's on the user, as they should only ever extend the state with something that can logically extend the state.

@timdorr timdorr merged commit 066fa81 into reduxjs:ts-conversion Sep 3, 2019
Comment on lines +274 to +275
StateExt = never,
Ext = {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, would it be possible to make StateExt and Ext optional whenever not using an Enhancer?
At the moment createStore<S,A,StateExt,Ext> generic typings are required as opposed as createStore<S,A> for stores without enhancers. Thanks!

webMasterMrBin pushed a commit to webMasterMrBin/redux that referenced this pull request Aug 21, 2021
* fix replaceReducer with a store enhancer

* remove erroneous restriction on StateExt

* remove the other extension - our store enhancer might add array functionality, for instance

* add reasonable defaults for Ext and StateExt

* fix state, add a test for non-object-based state

* add verification that store extension is also passed to replaceReducer

* better fix: set state default based on what base type it is

* fix array test

* fix typing of StateExt

* add mhelmerson example

* fix replaceReducer, so that it infers types, fix example test

* fix the weird type hacks in the test

* add final working example

* update based on PR type changes

* fix type

* update tests to reflect complete examples

* merge the changes from index.d.ts into types/store.ts

* extend store type

* much better approach: only extend the state when we have an extension

* fix typing issues not caught before

* add link to the place I learned about this


Former-commit-id: 5e5ac68
Former-commit-id: 6de326a
webMasterMrBin pushed a commit to webMasterMrBin/redux that referenced this pull request Aug 21, 2021
* fix replaceReducer with a store enhancer

* remove erroneous restriction on StateExt

* remove the other extension - our store enhancer might add array functionality, for instance

* add reasonable defaults for Ext and StateExt

* fix state, add a test for non-object-based state

* add verification that store extension is also passed to replaceReducer

* better fix: set state default based on what base type it is

* fix array test

* fix typing of StateExt

* add mhelmerson example

* fix replaceReducer, so that it infers types, fix example test

* fix the weird type hacks in the test

* add final working example

* update based on PR type changes

* fix type

* update tests to reflect complete examples

* merge the changes from index.d.ts into types/store.ts

* extend store type

* much better approach: only extend the state when we have an extension

* fix typing issues not caught before

* add link to the place I learned about this


Former-commit-id: 5e5ac68
Former-commit-id: 6de326a
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.

4 participants