Skip to content

[Docs]: Migration from mature Redux codebases and ideas for complex reducers cases that toolkit doesn't handle #685

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

Closed
antmelnyk opened this issue Aug 8, 2020 · 16 comments

Comments

@antmelnyk
Copy link

First of all, big thanks to the Redux maintainers. It's very nice to see that toolkit is very well received and Redux itself is well and "alive" despite some other cool kids on the street.

For our project, Redux did very good job making state centralized and predictable and we are quite happy with it, although we don't even use React or even HTML, we use it for a custom library that uses canvas to build the UI.

Continuing the discussion from here. Basically, would be very nice to see a more sophisticated "Migration" process for older and mature projects, or at least some insight about "is it worth it". Probably, related to the #558

Consider these cases:

  • Root reducer does not just call other reducers for corresponding state properties (like combine reducers) but has some ordering of the reducer calls and some conditionals for them:
const rootReducer = (state, action) => {
  const firstCycleState = {
    ...state,
    something1: reducer1(state, action),
    something2: reducer2(state, action),
    // ...
  }

  const secondCycleState = {
    ...firstCycleState,
    something3: reducer3(firstCycleState, action),
    something4: reducer4(firstCycleState, action),
    // ...
  }

  const thirdCycleState= {
    ...secondCycleState,
    something5: reducer5(secondCycleState.conditional ? otherReducer(secondCycleState, action) : anotherReducer(secondCycleState, action), action),
    something6: reducer6(secondCycleState, action),
    // ...
  }
  
}
  • Some of the reducers are written by other people and are passed to some "API" reducers that handle them in a specific way. Basically, high-order reducers:
const featureReducerCreator = (state, action, ...argumentsThatAlterReducerBehavior) => (state, action) => {
  // Logic around addigional arguments
  // ...
  switch(result) {
    case 0:
      return featureReducer(state, action, someAdditionalArg);
    case 1:
      return featureReducer(state, action, anotherArg);
    default:
      return featureReducer(state, action);
  }
}

// Later, other people use it with different variations.
const mySpecificReducer = featureReducerCreator(action, state, mySpecificArg);
  • Some of the reducers DO NOT need Immer.js. They will intentionally reuse the old state. Yes, it's idiomatically wrong, but there are performance reasons regarding calculations and there is no need in strict undo/redo. Edge cases, but still it's there and as I understand won't be possible to use createReducer.

  • There are no real "slices", some reducers accept the whole state or specific part of the state, not just the part they will return.

At first glance, it seems like I will have to "fight" its API more than just writing functions and using pure TypeScript to handle those cases. It kinda feels like toolkit loses a bit of the initial simplicity of "reducers are just functions" and reducers become "special functions with specific API". Same thing regarding action creators and specifically createSlice.

Maybe it's time for "You might not need Redux Toolkit" 😄 I assume I'm not the only one who kinda encountered that problem. "Toolkit is nice, but is too opinionated and covers only simple cases. I will start to use it for a new project, but seems like there is no benefit in migrating existing codebase.".

@antmelnyk antmelnyk changed the title [Docs]: Migration from mature Redux codebases and ideas for complex reducers cases [Docs]: Migration from mature Redux codebases and ideas for complex reducers cases that toolkit doesn't handle Aug 8, 2020
@markerikson
Copy link
Collaborator

Hey, thanks for filing this.

So FWIW, you've always been able to mix and match how you write reducer logic, whether it be with "just" the Redux core, combineReducers, or RTK:

https://redux.js.org/recipes/structuring-reducers/beyond-combinereducers

RTK does allow you to just pass an object full of slice reducers as the reducer option and calls combineReducers automatically, but you can also pass a root reducer you set up yourself.

I talked about the "multiple top-level reducers" pattern in my post Practical Redux, Part 7: Feature Reducers.

I'm not quite sure what you mean by your comment of:

Some of the reducers DO NOT need Immer.js. They will intentionally reuse the old state.

Can you give an example?

Immer returns the existing state if you don't make any edits.

Overall, RTK simplifies the 80+% use cases, and a lot of the stuff you're listing is outside that 80%. However, you can mix and match reducers written with RTK with your other special-cased reducers, and I haven't yet seen anything in your description that says you can't use RTK.

(Also, side question: for the "API reducer" example you gave, have you considered putting the "additional arg" into the action objects themselves?)

@antmelnyk
Copy link
Author

antmelnyk commented Aug 13, 2020

I talked about the "multiple top-level reducers" pattern in my post Practical Redux, Part 7: Feature Reducers.

Oh, thanks a lot for this! Somehow I skipped that part, but I've checked the whole Practical Redux, very cool reading. Well, basically yes, we reimplemented reduceReducers 😢 but is hardcoded to our needs.

Immer returns the existing state if you don't make any edits.

So for example if I have an array with 10000 items and I do mutation array[N] = value instead returning new array via map and one item changed, under the hood Immer will create a new array? These are concerns. Based on Immer performance tests without using Proxy that might be not good for performance.

have you considered putting the "additional arg" into the action objects themselves?

Well, this argument is a function in some cases and contains specific logic, so makes more sense to do it like that.

Overall, RTK simplifies the 80+% use cases, and a lot of the stuff you're listing is outside that 80%. However, you can mix and match reducers written with RTK with your other special-cased reducers, and I haven't yet seen anything in your description that says you can't use RTK.

Yeah, I figured we probably will try to implement it gradually for simple reducers and see how it goes, keeping use of "pure" Redux for edge cases.

@markerikson
Copy link
Collaborator

So for example if I have an array with 10000 items and I do mutation array[N] = value instead returning new array via map and one item changed, under the hood Immer will create a new array?

Yes, because the point of Immer is to do immutable updates, and that's how immutable updates work :) It's the same thing as if you wrote it by hand, just automated, and with a small overhead due to use of Proxies. How much that overhead actually affects your app, I don't know.

If you're actually mutating data in your reducers, then that's an abuse of Redux and wrong, and you really shouldn't be doing that in the first place.

FWIW, reducers are rarely the bottlenecks in a React-Redux app. The cost of updating the UI is almost always much higher. That's not to say reducers are never slow, as of course it always depends on your specific app architecture, state structure, and update patterns. But, I'd suggest actually trying it out and running some perf profiles to see what happens in both dev and prod.

@akutruff
Copy link

akutruff commented Aug 24, 2020

Just want to add a +1 the many "make immer.js optional opt-out/opt-in". I really want to use the great type inference, use official practices, and also the ridiculously clean approach to architecture.

I'm writing a simulator for financial applications, and just ran into immer taking 30ms out of 40ms of execution, and then using the pure reducer without createReducer() it dropped to 8ms. (I saw in the other issue that, yes, I can still define pure reducers with the toolkit.) Though the pure reducer approach works, it then precludes me from also using configureStore() and createSlice(), (They do also introduce immer for those reducers, right? Sorry I'm new here.) Note, I'm still being completely immutable in our case.

I understand that this library is supposed to be opinionated, and agree with the philosophy generally. I will challenge the 80% philosophy here though as it can lead to forks... If I can't figure out how to use configureStore()/createSlice() without producing immer proxies, then I'll sadly have to fork. It's too risky otherwise. The unfortunate part, is that the fork will simply be "redux toolkit with immer optional."

@phryneas
Copy link
Member

@akutruff have you measured these times in a production build? What takes most time with immer is the autofreezing and that is only enabled as a security measure in development mode.
Also, you will likely be running into performance problems using "normal" object spread as well. 8ms is still a lot of time (I'm guessing thousands of manipulations?). In this specific case I'd recommend actually the immutable.js route as they're doing performance optimizations that make them well faster than plain JS code. Best part of it: it should work just fine in RTK reducers, as immutable.js objects are non-immerable and the reducers just fall back to "normal reducer" behaviour in that case.

@markerikson
Copy link
Collaborator

markerikson commented Aug 24, 2020

Note that:

  • In dev mode, we're also doing deep checks for accidental mutations and serializability, which is another reason to profile against prod
  • configureStore has nothing to do with use of Immer. It does turn on the dev check middleware.
  • The one valid use case for use of Immutable.js at this point, that I can see, is needing to make updates to huge JS objects/arrays (10K+ keys/items)

As Lenz said, first try profiling in a prod build. If things are actually slow, it'd help if you can provide a CodeSandbox that reproduces the issue for us to look at.

But no, we're not making Immer optional. Worst case, you hand-write specific reducers for specific scenarios.

@phryneas
Copy link
Member

@akutruff also, since you have an edge case, could you measure again with the build from #698 and report back if it has significant performance differences please? If so, we might consider making that part of it configurable, but so far we have not seen any reports that it actually made a difference.

As for forking: I don't think that is something we should be afraid of. I'd rather have 10 people with a valid use case copy one of our tools into their code base & use the rest of RTK as it is than 50-100 users disable immer just because is is possible as a premature optimization and suffer down the road.

@akutruff
Copy link

Thanks for the reply! I did profile under production in Chrome after being warmed. For Immutable.js Eh... I'm weary of using yet another library with it's own quirks, and overhead that I don't need in order to turn off the usage of another library. It appears I have TypeScript's ReadOnly<>, and unit tests which get the job done.

I'll see if I can try that build.

@phryneas
Copy link
Member

@akutruff yeah, the point of my suggestion was primarily that it is actually faster than most things you can do in plain JS by quite a bit as the internal data structures are more suited for immutable operations than javaScript's arrays & objects.
If you can avoid it, good. But once plain JS gets too slow keep in mind that you still have that option. But it's a bit fiddly to write, I'm usually not a fan of it if I can avoid it.

@markerikson
Copy link
Collaborator

@akutruff : yeah, if you could provide a CSB repro that shows the perf issue, and possibly a Chrome performance profiling export from your specific app, that'd be helpful.

@akutruff
Copy link

@markerikson @phryneas was so very kind as to look at this offline with me. I put up this repo showing the cases.

https://github.com/akutruff/conway-life-react-redux

The TLDR; - if you want to have a reducer that access a bunch of properties and you want to cut out immer - call your reducer with original() and it will be up to you. It still allows you to use createReducer and all the other goodness in the toolkit. Glorious.

@phryneas
Copy link
Member

phryneas commented Aug 24, 2020

TLDR2: Game of life is a really unfair benchmark for immer, as every (naive) iteration on a 100x100 board modifies 10k cells and accesses each cell multiple times 🤣

@markerikson
Copy link
Collaborator

markerikson commented Aug 24, 2020

Hey, that does qualify as a nice stress test :)

Good to know about the use of original(). We should document that officially, and then go back through the other "make Immer optional" threads and point to that docs entry.

@phryneas
Copy link
Member

Yeah, to follow up on that, I did some immer code reading.

Immer can get expensive even if you return a new object in two situations:

  • accessing proxified state.
    This can be circumvented by using original(state)
  • returning a very big new state, as that will go through finalize
    This seems to be circumventable by returning a frozen object. But we had no significant slowdown in the finalize phase anyways. Probably this is only a problem on deeply nested structures.
    Deep-freezing is not necessary, as this is only a means to prevent immer from recursing deeper into the tree.

So this leaves this as an optimal "circumvent immer in RTK" usage:

const slice = createSlice({
  name: "foo",
  reducers: {
    normalReducer(state, action) { state.foo = action.payload.bar },
    dodgyReducer(state, action) {
      const oldState = original(state);
      // ...
      return Object.freeze(newState)
    }
  }
})

@akutruff
Copy link

TLDR2: Game of life is a really unfair benchmark for immer, as every (naive) iteration on a 100x100 board modifies 10k cells and accesses each cell multiple times 🤣

Don't hate the player, hate the game? =D

Seriously though I actually chose it as a toy version of my use case: A lot of time series data being mapped to a lot of other time series data. Imagine for example, all financial instruments in the stock market getting updated each simulation step based on some financial model. This is a case of using redux in a webworker rather than in a react use case.

@markerikson
Copy link
Collaborator

This thread's been sitting idle for a while. My takeaway at this point is there's not much in the way of concrete actionable steps for us to take here, so I'm going to go ahead and close it. Please let me know if there's something specific we can follow up on!

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