Fix type of next parameter in StoreEnhancer type #3776
Conversation
|
Deploy preview for redux-docs ready! Built with commit c5863d5 |
|
Deploy preview for redux-docs ready! Built with commit 92cf2a2 |
test/typescript/enhancers.ts
Outdated
| function replaceReducer<NewState, NewActions extends Action>( | ||
| nextReducer: Reducer<NewState, NewActions> | ||
| ) { | ||
| store.replaceReducer(nextReducer) | ||
| return newStore | ||
| } |
There was a problem hiding this comment.
Many of the enhancer tests had incorrect replaceReducer methods. Previously they were actually returning the old store without the enhancer extensions applied instead of returning the new store with the enhancers applied. The fix in the types caught this bug.
There was a problem hiding this comment.
Also note that the boilerplate that is now required for any enhancer is significantly larger since replaceReducer has to return the extended store. If #3772 gets merged, this boilerplate would be reduced significantly.
| newStore.getState().wrongField | ||
| } | ||
|
|
||
| function composedEnhancers() { |
There was a problem hiding this comment.
This is a new test that would fail without this fix.
|
Can you fix the conflicts? |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 5a084a6:
|
|
I'll try to circle back and actually look at this in the near future. My main observation atm is that this is gonna go onto |
|
@markerikson No problem, don't mean to guilt you into looking into this but the old types are objectively wrong and so I'd love to get this merged. I'd be happy to create a separate PR to target 4.x. |
|
@markerikson Also #3772 really needs to get merged at some point as well. 😉 |
|
Let me know when you're ready to look at either one and I can resolve merge conflicts. |
src/types/store.ts
Outdated
| export type StoreEnhancer<Ext = {}, StateExt = {}> = <NextExt, NextStateExt>( | ||
| next: StoreEnhancerStoreCreator<NextExt, NextStateExt> | ||
| ) => StoreEnhancerStoreCreator<NextExt & Ext, NextStateExt & StateExt> |
There was a problem hiding this comment.
This is the main change. All the other changes in this PR just support this.
There was a problem hiding this comment.
Not sure why it doesn't show the old lines but here it is for context:
export type StoreEnhancer<Ext = {}, StateExt = never> = (
next: StoreEnhancerStoreCreator<Ext, StateExt>
) => StoreEnhancerStoreCreator<Ext, StateExt>
|
I'd like to try to do some general Redux org issue triage and cleanup over the next couple weeks, so yeah, getting this knocked out would be high on the priority list. |
Methuselah96
force-pushed
the
next-ext-2
branch
5 times, most recently
from
2a304f3 to
abcad5b
Compare
Methuselah96
force-pushed
the
next-ext-2
branch
from
abcad5b to
5a084a6
Compare
3 participants
name: "Fix type of next parameter in StoreEnhancer type"
about: Fix type of next parameter in StoreEnhancer type
PR Type
Does this PR add a new feature, or fix a bug?
Fix a bug. Resolves #3768.
Why should this PR be included?
Without this PR the type for
nextinStoreEnhanceris incorrect. It assumes thatnextwill create a store with the current enhancersExtandStateExtalready applied to store. Instead,nextreturns a store withNextExtandNextStateExtgeneric parameters since the enhancer should work when composed with any other enhancer. It then expects theStoreEnhancerto return a new store creator that will create a store withExtandNextExt, andStateExtandNextStateExt.Note that I had to remove the
ExtendStatetype in order to make this work. This type was introduced in #3524, but I wasn't able to get the types working while usingExtendState. I think the types are more clear without it anyway.Checklist
Bug Fixes
What is the current behavior, and the steps to reproduce the issue?
Steps to reproduce are in #3768. There's also a new test for checking the types on stores created from composed enhancers. This test fails in
master.What is the expected behavior?
The types should allow for a generic
NextExtandNextStateExt.How does this PR fix the problem?
It adds those generics.