RFC: Make immer optional for createReducer
#242
Comments
|
Already asked in #183 , and no, I'm not planning to do this. The point of RSK is to be opinionated. There's a million There's related discussion at #90 (comment) . Immer has a global I can hypothetically imagine wanting to set that on a per-slice/reducer basis, and it would be technically possible if we accepted an option and then created a unique In general, my recommendation is to either not use RSK's functions here, or disable Immer's auto-freezing. |
|
I'm really on the fence about using RSK because of this whole Immer thing. We don't use it or Immutable.js at my company and we have a ton of junior devs writing Redux code. They don't have any problem just using spread and destructure to write immutable state reducers. The things being done in this library are very similar to what we have for our own in-house library but I wanted to switch to RSK because it's an official, well documented and maintained kit. If we use RSK and continue to write reducers that always return a whole new state object, Immer will be doing nothing for us but adding to our overhead and bundle size. Looking at Immer releases, I see their still making some major changes and fixing bugs every release so it's a little scary to me. I guess I just wanted to add my voice to support for this option or something like it - maybe a separate non-Immer implementation of |
|
As I said earlier, the point of RTK is to be opinionated. Sure, you can write nested immutable update logic by hand, the way you always have. In fact, you still can while using RTK, because you can return new state values from reducers. But the point is you don't have to do it the hard way, and you shouldn't because it's error prone. RTK currently uses Immer 4.x, as the 5.x releases don't add any features that we care about (specifically If you skip the Immer aspect, RTK's function createReducer(initialState, handlers) {
return function reducer(state = initialState, action) {
if (handlers.hasOwnProperty(action.type)) {
return handlers[action.type](state, action)
} else {
return state
}
}
}But seriously, you should be using Immer, via |
I don't think that using spread/destructure is hard or error prone. As a matter of fact, picking out errors is easy because the pattern is very clear and when you don't follow it - it sticks out like a sore thumb in code reviews. We don't put a lot of logic in our reducers. I honestly can't remember the last time we had an issue with reducers and we have dozens of teams using React/Redux.
Yep. I hear you - that's why our in-house kit already has something like |
Well, I'm genuinely glad your teams are doing well :) But having seen hundreds of Redux apps and helped thousands of users, I can confirm that accidental mutation is by far the most common mistake that users make, and I can also confirm just from my own experience that writing manual immutable update logic is a royal pain. I don't ever want to have to do that again, I don't want our users to keep having to deal with that, and the size and abstraction Immer adds are totally worth it for the way it makes the rest of my code simpler.
Note that this doesn't actually make things better or easier. If you're doing something like generating a new array outside of Redux in a component, and your reducer is just That's one of the reasons why the new Style Guide page specifically recommends that you should put as much logic as possible in reducers. It helps remind you that you should be following the rules of reducers like immutable updates, and if you're using RTK, you get the benefit of Immer for doing those updates. |
|
To add to this:
immer does not only protect you from accidental modification in the reducer, but also from accidental mutation during render (and everywhere else). And yes, you might catch those in code reviews, but that code review is bound to happen when code is submitted. Most likely someone already spent hours chasing down some bug somewhere before that review. That a bug is caught doesn't mean that it didn't waste quite a few developer hours beforehand. Also, as Mark already said: The bundle size of immer is smaller than the amount of code that immer saves you in almost every project that is large enough to worry about bundle size. |
|
Hmmm, well you guys make some good points and even though we don't generally have problems with the things you're describing, it might not be bad to protect against them anyway. Maybe there are problems that haven't even caught yet that can be fixed.
In 2 years of using Redux I think this happened to us once or twice. They would tell me. I'm not saying it's not a problem ever and it's certainly not a fun bug to deal with, but the main thing I'm afraid of is that we trade that relative safety for new problems. I'd hate to recommend a pattern switch and then have it cause more problems than it solves. So, I'm just reading through the issues here and on Immer to see what people are going through and as far as I can see, it's not all risk free. (I'm a little nervous that there's no redux-persist guidance because that is one thing we have spent hours perfecting in our current setup. I saw the issue where the one commenter said they solved it so maybe it will just work out.) At this point I guess I'll setup a trial project using RTK and see what happens, discuss with my peers and so forth... Thank you so much for your detailed and thoughtful replies! |
|
If you notice any bumps on the road, make sure to open a ticket. |
|
@waynebloss : yeah, while I haven't ever used There's no "guidance" on using that in the docs, simply because it's a purely optional addon that isn't related to the Redux core or RTK at all. I could see us possibly adding a new core docs page on "Store Persistence" at some point, but that's about as far as I'd go. |
|
I wouldn't add There is performance overhead with I do think making |
|
@markerikson Well since we're here - what do you personally do for persisting a slice of state if you've never used (I actually checked how widely used |
|
Another reason to not bother making Immer optional atm is that we still don't have tree-shaking working for RTK, due to other dependencies: #78 . So, if you use RTK, right now you're getting Immer whether you use (I'd really like to improve the tree-shaking behavior, but my priority atm is rewriting the Redux core docs, and no one has jumped in and tried to fix the build behavior.) Folks are always welcome to fork the lib and do whatever they want with it, but given that I'm the maintainer, I'm the one setting the vision and what I want in the lib. @waynebloss : I've never had to bother with store persistence, or routing for that matter. The apps I've worked on have been true "single-page" apps - ie, desktop-like apps that just happen to live in a browser. |
This is a little bit of a rant, and even though it was triggered by you, it's more of a "we really have to have this written here" thing, so please don't take it as an attack 😄 If anyone has performance problems, immer is probably the last place to lookFirst about the comparison with deep-freeze: So, aside from the performance impact of preventing mutation, there's one other point where immer can have a performance impact:
If you have done all that and still suspect immer of "slowing down" your application, you might be one of those rare cases that has deeply-nested lists with tens of thousands of object in the store. Then removing immer isn't going to save you, because even vanilla js will be slow for you - you're gonna have to go the immutable.js route then, I'm sorry.
Yup, just had to say the same thing with a lot more words for everyone to read 🤣
It would duplicate a lot of the code and the types of RTK (which would be a chore to maintain) and it would signal that we think that using Redux without immer is a good idea - which I would not ever recommend if the option of immer were in the room. Sure, there may be some extreme edge cases (where you would need something like immutable due to very big data structures) but the people that need these know it themselves. |
|
I would like to disable I'll definitely check out |
|
@theogravity serializability is a redux concern, not an immer concern. Non-serializable objects do not work with many middlewares and the devtools, which is why we warn about them. But you can always disable those warnings manually. |
|
@theogravity I'm kinda confused - if you're not using Redux, why is RTK even involved here? |
|
@phryneas Thanks for the reference! @markerikson The project was not built on redux and has its own state management scattered around and I am migrating it to use redux (RTK specifically) as we have complex data flows that are difficult to debug and to reduce future tech debt. |
|
Big fan of RTK, but I just wanted to throw in another use case where the forced use of immer is causing issues. I have a reducer where all the actions/handlers use immer, except for one. That one needs to accept incremental patches incoming from the server. I can build these patches in a way that’s compatible with immer’s applyPatches functionality, but because RTK wraps everything in produce I can’t easily apply the patches. Right now I’m using fast-json-patch from inside a reducer, which seems ridiculous, and it’s very difficult to debug due to the proxies. I’m thinking I will either have to rewrite all the affected reducers and use produce manually, or make use of immer’s new “original” function and return a new value just to break out of the box RTK has put me in. Just because something is opinionated doesn’t mean it shouldn’t have escape hatches. |
|
@trappar I've already said, this thread and others, why we're not going to make Immer optional for That use of "patches" sounds like a very rare edge case to me. RTK is built for the 80%+ use case. If you have cases where you need to do something different, you can always write a separate reducer by hand. |
|
For others who find themselves in a position like this - here's a 12 line reimplementation of my usage of this library. This offers an API almost identical to RTK but also supports reducers that aren't wrapped in immer's "produce". Think of it as a jumping off point for your own implementation: import {produce} from 'immer';
export const createReducer = ({initialState = {}, immerReducers = {}, reducers = {}}) =>
(state = initialState, action) => {
const type = action.type;
if (immerReducers.hasOwnProperty(type)) {
return produce(immerReducers[type])(state, action);
} else if (reducers.hasOwnProperty(type)) {
return reducers[type](state, action);
}
return state;
} |
|
Just to state this once more as people generally seem to forget about it: Using RTK doesn't mean you can't write your own reducers. RTK offers shortcuts, you don't have to use each and every one of them at all times and all costs. A simple example for a one-off reducer combining one non-immer reducer with a RTK slice: const reducer = (state = initialState, action) => {
if (oneOffAction.match(action)) {
return ...// custom logic here;
}
return mySlice.reducer(state, action)
}Also, from RTK 1.4 forward we are checking with immer's
So if you have something non-serializable, there is a great chance that it isn't wrapped in |
|
I'm bypassing import { original } from 'immer';
import { merge } from 'lodash';
const API = createSlice({
name: 'API',
initialState: initialState(),
reducers: {
fetchResources: (draft, { payload }: FetchAction) => {
const state = original(draft) as ApiState;
return {
...state,
objects: merge(
{},
state.objects,
normalize(payload.response, normalizeOpts)
),
};
},
}
};) |
|
That's not "bypassing" Immer. The complaints here have been centered around Immer's |
|
I think the authors of RTK have consistently made very solid decisions, and I would rank using immer as one of the best of these decisions. I love RTK; I love A specific use caseI am here, to share a specific use case that might be of interest. I'm working on a Webpack plugin that hooks on many compiler and compilation events. These events are dispatched as actions to the store. For the app produced by 0.2% immer penaltyIs this something to cause an immer performance concern? Immer's performance shows that updating 5000 items (out of 50,000) takes roughly 19ms. So in my case, we are looking at about 2ms per second immer "penalty" (not science, just crude approximation). But still...Still, all of this happens in node, with no requirement whatsoever for immutability. Hence I landed on this issue. (The browser part of the plugin uses the same store and DOES require immutability into React.) So whilst I understand the rationale behind not providing the option to opt out of immer, and I think a very strong case was made for that above, I just feel this specific case should be known. |
|
Heh. Using RTK in a Webpack plugin is certainly not one of the primary use cases we're targeting :) Obviously it can work, because a Redux store and reducer functions are just plain JS and can be used anywhere. But in terms of "use cases RTK is specifically designed for", it's pretty far off our radar :) |
|
Indeed, it's really not a typical RTK/redux case. I do want to note the following though: Redux as a Reactive FrameworkRedux (and hence RTK) are pitched and universally seen as a state management solution. But personally I've found it to be a very powerful reactive framework. I've harnessed its reactive potential in a variety of projects and submodules (to give one example, a game-server simulator to drive integration tests), and I cannot think of a design/technology that would lend itself better to the problem at hand. It is a sort of 'just-right' reactive framework. Compared to, say, RxJS or cycleJS, redux is more mainstream, scoped, simple - and it does what you want it to do. RTK has nothing to do with this directly (all the reactive part is due to middlewares), but there's always an element of a store involved. RTK to RuleRedux was designed as a high-level abstraction. As such it offers flexibility over usability. RTK targets the concrete, so we have usability over flexibility. RTK is well-thought-out, well-scoped, and a direct offspring of redux. Although I'm sure I haven't been exposed to every use-case out there, I'm struggling to come up with an example where redux would be more sensible than RTK. The ProphesyI'm just saying this because there is some chance RTK will be another case of 'a victim of its own success' - it may get such a phenomenal traction (if it isn't already), that use-cases and requests are just going to grow. RTK has a very well defined scope and use cases in mind - for a good reason. Just a prophesy of the difficulties that may lay ahead. Anyhow, this is all well off topic, so let's not continue this discussion here. |
|
It's sad to see how many people doesn't understand the point on writing immutable code. Many people only do it because react and redux require it, and that's all. Libraries like immer keep people in the ignorance at the same time they support old bad habits of coding at the same time it adds bundle size, performance penalty and impossible to polifyl requirements . Immutable code is better because it is easier to test, easier to follow and easier to reason about. JS has enough primitives to write immutable code easily, I don't understand why experienced programmers keep wanting to use |
|
@danielo515 I really don't understand what point you're making here. The important thing is that the data gets updated immutably. The syntax for that and how it is accomplished is not what matters. Now, yes, I get the point of needing to know what immutability is and how to do it by hand, so that you understand what Immer is actually doing for you. That's why I've repeatedly explained what immutability is, what Immer does, why this matters, in many many places throughout our docs:
Yes, and those primitives are hard to use and very error-prone. Accidental mutations have been the number one cause of bugs in Redux apps since day 1. Immer not only lets you write shorter code, Immer effectively eliminates that source of bugs. That right there is enough reason to use Immer as a default. If you still want to write immutable update logic by hand, you can, even in
This is incorrect. Immer has an ES5 fallback that works if there are no proxies available in the environment, and RTK already turns that on by default.
This is simply wrong, and all the other feedback I've gotten has proven that the current RTK design is the right way to go. |
|
At this point I don't see any reason to keep this thread open, because we're not changing the design. |
I think end-developers should be able to opt-out of
immerbehavior forcreateReducerandcreateSlice. There have been some questions raised recently after the 1.0 announcement that people have experienced performance issues when usingcreateSlicebecause ofimmer.For most use-cases, I think immer should be enabled. However, there are specific cases where I have needed to tune the performance of a set of reducers and needed to disable immer.
immer definitely provides a ton of value out-of-the-box and I think should be enabled by default. However, I also think this is one area where we can be a little more flexible and allow the option to disable it.
createReduceris still extremely valuable even without immer and I think that is the main reason why I think it should be opt-out.For
createReducerwe could provide an additional parameter that determines if immer is used:Or we could create a separate function
I'm curious of everyone's thoughts. Can anyone else think of other ways to implement this feature?
I also think that
createSliceshould provide a propertyimmerwhich gets passed tocreateReducer.const slice = createSlice({ name: 'slice', + immer: false, // ... });I could see this kind of change requiring some typings work before merging.
I'm willing to do the work to get a PR up for this, but I wanted to make sure it was something that the maintainers agree would be useful.
The text was updated successfully, but these errors were encountered: