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

Make immer-based createReducer more explicit #123

Closed
tlrobinson opened this issue Mar 15, 2019 · 5 comments
Closed

Make immer-based createReducer more explicit #123

tlrobinson opened this issue Mar 15, 2019 · 5 comments

Comments

@tlrobinson
Copy link

tlrobinson commented Mar 15, 2019

I really like the idea of integrating immer into a createReducer-like function, however I feel like it's a bit too much magic to be simply called createReducer and would prefer if it were more explicit, something like createMutableReducer or an opt-in option like createReducer(..., ..., { mutable: true }).

I see in #5 one of the motivations for including immer is to "address the issue of users having accidental store mutations because they don't know that Redux requires immutable updates". It seems like quietly including this feature only exacerbates this problem in the long run. If a new Redux users learns with redux-starter-kit and later works on a project that doesn't use immer they are highly likely to mutate the store. I think a better solution to that problem is to use something like https://github.com/leoasis/redux-immutable-state-invariant in development mode.

So I'm proposing two things:

  1. make the use of immer more explicit in the createReducer API
  2. include redux-immutable-state-invariant or something like it in the default middleware in development mode

Alternatively, at a minimum I think renaming state to nextState in the docs (as suggested here) would be wise.

@markerikson
Copy link
Collaborator

We actually already do include redux-immutable-state-invariant as a default middleware in development :)

https://redux-starter-kit.js.org/api/getdefaultmiddleware#development

I agree that Immer adds "magic", and that it's not obvious just by looking at code that the state isn't really being mutated. Overall, I don't have a good answer for that, other than documentation (which we already warn about in the "Usage Guide" docs page and the Redux docs "Immutable Update Patterns" page.

I do see where you're coming from with suggesting that the "mutation" be a bit more opt-in, and I'm not going to say you're wrong about it. But, part of the point of RSK is to add opinionated defaults on top of Redux. Given that, I'd prefer not to make that something that's configurable.

@donysukardi
Copy link

I really like the idea of having immer as an opt-in instead.

@markerikson
Copy link
Collaborator

As I said, most of the point of RSK is to offer opinionated defaults. If you don't want them, don't use RSK - just do whatever you've been doing already.

@sebqq
Copy link

sebqq commented Aug 28, 2019

@markerikson

Now in RN there is hermes engine for android devices which is not supporting Proxies and maybe it will never be. Doesn’t immer.js have negative impact on performance if it has to be somehow polyfilled or something?

@markerikson
Copy link
Collaborator

Immer has an ES5-compatible fallback if Proxies aren't available. That's supposed to be a bit slower. But, in general, reducers are not the bottleneck - updating the UI is.

markerikson pushed a commit that referenced this issue Apr 20, 2021
* make prepareHeaders support async functions
* add test for async prepareHeaders function

Co-authored-by: Dennis Schaller <dennis.schaller@klarna.com>
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