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

replaceReducer eliminates enhancer #4127

Closed
antontsvil opened this issue Jul 6, 2021 · 14 comments
Closed

replaceReducer eliminates enhancer #4127

antontsvil opened this issue Jul 6, 2021 · 14 comments

Comments

@antontsvil
Copy link

What is the current behavior?

If you use an enhancer when creating your Redux store, the first subsequent call to replaceReducer on the store seems to remove the enhancer.

Steps to Reproduce

Run this JSFiddle and observe the console
The enhancer will stop logging actions following the call to replaceReducer

What is the expected behavior?

Not sure if what I'm reporting is not a bug and I'm misunderstanding the usage of enhancers; But seems to me enhancers should persist for the lifetime of the store, regardless of which reducers are being used.

@timdorr
Copy link
Member

timdorr commented Jul 6, 2021

That isn't an enhancer. It's just a wrapper function on createStore. To create an enhancer, you would need to change something about the store itself, such as replacing the replaceReducer function on the resulting store.

@timdorr timdorr closed this as completed Jul 6, 2021
@markerikson
Copy link
Contributor

Mmm... it looks like a valid enhancer in terms of syntax, at least. But yeah, the real point of an enhancer is to override one of the actual store methods. All this is doing is wrapping the original reducer function. So yes, if you replace that reducer, you've... replaced the reducer :)

@Methuselah96
Copy link
Member

Methuselah96 commented Jul 6, 2021

This is actually a bug in your enhancer. If your enhancer messes with the reducer, it needs to re-implement the replaceReducer function (and this is actually something types won't catch if you happen to be using TypeScript). See https://github.com/reduxjs/redux/pull/3776/files/b6483652e8d68663bf355061bc2b34a63c09e978#diff-326d6e3ed223aeeb0f5f2099b79d5910860494677628d63c13b2ff3bb223ae3eL14 for an example. Never mind, this scenario is slightly different, that example only applies if you're using the returned store from replaceReducer which isn't released yet. https://github.com/reduxjs/redux/pull/3776/files/b6483652e8d68663bf355061bc2b34a63c09e978#diff-326d6e3ed223aeeb0f5f2099b79d5910860494677628d63c13b2ff3bb223ae3eR94-R113 is probably more close to your scenario. However the general concept still applies because you're changing the reducer in your enhancer, so you need to re-implement replaceReducer.

@Methuselah96
Copy link
Member

Methuselah96 commented Jul 6, 2021

P.S. @timdorr @markerikson It would be great to get #3776 (and #3772 🤞) merged, so that the folks using TypeScript will catch this bug in their enhancers sooner.

@Methuselah96
Copy link
Member

Methuselah96 commented Jul 6, 2021

@antontsvil Here's a fiddle showing how it should be implemented: https://jsfiddle.net/nk3bqcfv/.

@markerikson
Copy link
Contributor

@Methuselah96 the issue with those PRs is that they're against master, which is the still unreleased "convert all of Redux to TS" codebase, which we also have no timeline to release atm.

In order to actually be releasable any time soon, we'd need updates made to the TS typedefs in the 4.x branch.

@Methuselah96
Copy link
Member

Methuselah96 commented Jul 6, 2021

@markerikson I understand, I can re-implement #3776 for 4.x (#3772 reverts a change that's only in 5.x). What are the breaking changes in 5.x? Are the types different enough that it needs a new major? What's holding it back from getting released?

@markerikson
Copy link
Contributor

That's sorta the issue - we're not sure what the breaking changes are, it's likely the types would benefit from more changes, and there's not enough reason to push it live and cause churn in the ecosystem atm :)

The conversion work was actually done in... late 2019, I think? And it's been sitting there in master ever since.

Basically, the community contributed the work after we asked for it, and then the whole effort just stalled.

There's also some tie-in to redux-thunk's types being stuck in a holding pattern as well.

Basically, what's live in 4.x right now works well enough, and especially with RTK now being the default, that there just isn't enough pressing reason to try to push through a 5.0 release any time soon.

@Methuselah96
Copy link
Member

Yeah, makes sense. Sorry to derail this issue. FWIW I have annoying casts in my code that could be removed if reduxjs/redux-thunk#255 were released. It's frustrating seeing completed work from 2 years ago that fixes issues that I'm dealing with that has no timeline for a release. (I could potentially augment the interface, but I was kinda hoping it would just get released at some point.) These situations make DT seem a lot more appealing so the types lifecycle don't have to be as tightly coupled to the JS releases, but I understand that decision has a lot of pros/cons and has been hashed and rehashed many times and frankly I'm not sure which I prefer.

But I also understand that keeping churn down is valuable, so thanks for the hard work in that regard.

@Methuselah96
Copy link
Member

Methuselah96 commented Jul 6, 2021

(And I'm also not on RTK yet, as noted in reduxjs/redux-thunk#299 which contributes to the problem on my end.)

@markerikson
Copy link
Contributor

Yeah, I'd like to see 5.0 released too, but it needs some meaningful attention to figure out what other types changes might be necessary, and given everything else going on it's just at the bottom of the priority list for us right now.

If someone else with serious TS knowledge wants to help us out and try to push the current TS port forward by evaluating it and seeing where things stand and how it can be improved, I'd love it.

@markerikson
Copy link
Contributor

Opened up #4129 to cover that topic.

@antontsvil
Copy link
Author

Thanks for pointing out what I figured was the initial issue, was that I wasn't understanding enhancers 100%. And thank you for your helpful solution @Methuselah96. I do have to note that I had to modify it because your solution never actually uses nextReducer. Which means that the enhancer will in fact continue to run, but operate with the old reducer. What I've done to resolve this is pass the new reducer to the enhancedReducer method and use the initial reducer as a fallback.

This seems to fix my issue (enhancer which supports replaceReducer AND operates using the new reducer) but I'm not sure if it's an antipattern, looks a bit weird tbh. Just leaving the solution here for anyone else encountering the same problem.

@Methuselah96
Copy link
Member

Methuselah96 commented Jul 6, 2021

Good catch. It is a bit odd looking. I would go with a slightly different implementation to avoid the (updatedReducer || initialReducer), but it's still not pretty.

This issue was closed.
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

No branches or pull requests

4 participants