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 type #3507

Merged
merged 4 commits into from
Aug 16, 2019

Conversation

cellog
Copy link
Contributor

@cellog cellog commented Aug 16, 2019

Currently, the replaceReducer function assumes that the reducer state stays in the same shape. This does not make sense, as the purpose of replaceReducer is to change the store state.

However, typescript types are static, and replaceReducer changes the type at run-time.

To solve this issue, we either need to cast the existing store to the new store type, or transform the store.

As it turns out, a simple solution is to modify the signature for replaceReducer to return the existing store, and this gives us the ability to typecast to the new reducer type in a graceful way.

With this change, the combineReducers test passes when converted to typescript with minimal changes.

This may also fix #3482

@netlify
Copy link

netlify bot commented Aug 16, 2019

Deploy preview for redux-docs ready!

Built with commit 6b76c98

https://deploy-preview-3507--redux-docs.netlify.com

@netlify
Copy link

netlify bot commented Aug 16, 2019

Deploy preview for redux-docs ready!

Built with commit 500265f

https://deploy-preview-3507--redux-docs.netlify.com

@timdorr
Copy link
Member

timdorr commented Aug 16, 2019

We should probably add some tests against those new types.

@cellog
Copy link
Contributor Author

cellog commented Aug 16, 2019

We should probably add some tests against those new types.

yes, would that be in typescript.spec.js? nope. I hadn't looked first

@cellog
Copy link
Contributor Author

cellog commented Aug 16, 2019

@timdorr pushed a test, let me know if you think more is needed

@timdorr timdorr changed the base branch from master to ts-conversion August 16, 2019 16:04
@timdorr
Copy link
Member

timdorr commented Aug 16, 2019

That's a good enough start for me. Thanks!

@timdorr timdorr merged commit 2c1af36 into reduxjs:ts-conversion Aug 16, 2019
@timdorr
Copy link
Member

timdorr commented Aug 16, 2019

Oh crap, I missed that this was supposed to be against master. Welp, we can wait until the full TS conversion is done.

@cellog
Copy link
Contributor Author

cellog commented Aug 16, 2019

No worries, I only did it against master because that was one of 2 options, and this one is just fine too

@cellog cellog deleted the cellog-3482-replaceReducer branch August 18, 2019 16:26
webMasterMrBin pushed a commit to webMasterMrBin/redux that referenced this pull request Aug 21, 2021
* fix replaceReducer type

* remove extraneous type

* Update the JSDoc so IDEs don't complain.

* new test for replaceReducers


Co-authored-by: Tim Dorr <timdorr@users.noreply.github.com>
Former-commit-id: 65ca503
Former-commit-id: e90d501
webMasterMrBin pushed a commit to webMasterMrBin/redux that referenced this pull request Aug 21, 2021
* fix replaceReducer type

* remove extraneous type

* Update the JSDoc so IDEs don't complain.

* new test for replaceReducers


Co-authored-by: Tim Dorr <timdorr@users.noreply.github.com>
Former-commit-id: 65ca503
Former-commit-id: e90d501
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.

2 participants