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

Consider safeguarding against store mutations in dev #2858

Closed
nickmccurdy opened this issue Feb 28, 2018 · 9 comments

Comments

@nickmccurdy
Copy link
Contributor

@nickmccurdy nickmccurdy commented Feb 28, 2018

As per discussions in #135 and #138, it's helpful to have either frozen stores (https://github.com/buunguyen/redux-freeze) or mutation warnings (https://github.com/leoasis/redux-immutable-state-invariant) about mutation warnings to protect developers against common Redux issues caused by store mutations. While these middleware already work well, there's the issue that many developers new to Redux don't know that what immutable updates are, or don't know that Redux requires them and only does shallow comparisons of references. I also see a lot of developers asking for help because their Redux reducers don't detect updates because they accidentally perform shallow instead of deep clones, or developers experiencing performance degradations by deep cloning their entire store on every dispatch.

I propose enabling a package like redux-freeze, redux-immutable-state-invariant, or perhaps even a custom implementation by default in development. This would help solve the divide of developers that have issues with state mutation, but don't about the concept so they wouldn't know they needed to manually add a mutation warning library. Redux already makes some very helpful checks like combineReducer validation and plain object actions in development, and I feel this would be a nice additional for development environments.

@timdorr

This comment has been minimized.

Copy link
Member

@timdorr timdorr commented Feb 28, 2018

@timdorr timdorr closed this Feb 28, 2018
@jimbolla

This comment has been minimized.

Copy link
Contributor

@jimbolla jimbolla commented Feb 28, 2018

I don't think we should change core redux to add this. But the way I'd like to see it solved is to have something like a redux flavor of create-react-app that configures your store with some recommended store enhancers, with a mutation-detector as one of them.

@tkvw

This comment has been minimized.

Copy link

@tkvw tkvw commented Feb 28, 2018

I think this is dismissed very quickly. Mutating state is a common beginner mistake and it makes sense to have a warning about this when developing. I think the existence of these middlewares should be mentioned more permanently in docs at least.

👍 for @jimbolla comment.

@nickmccurdy

This comment has been minimized.

Copy link
Contributor Author

@nickmccurdy nickmccurdy commented Feb 28, 2018

I understand that there are performance and stability difficulties with checking for mutations in development by default, but in my opinion the improved development experience and educational value for helping new Redux developers is worth it. Whether or not this is possible with a tool enabled by default in Redux core's development mode, I would like to find a way to improve the development experience for accidental state mutations for Redux users (without external dependencies). I also feel this is something we could improve in documentation.

@timdorr and #1739

Object.assign() is shallow so it only copies one level deep. Copying deeper comes with a big perf penalty and disables any optimizations that we can get from comparing the references to check if something changed.

Redux already makes checks that have performance penalties in development environments. I think it would be worth it to do case studies to see just how bad the performance hit is and if we can improve it. I'd rather have production performance than development performance. Redux's shallow comparisons are fantastic for this but it's hard to have accurate reference checks without mutation warnings in development or requiring users to have a more advanced understanding functional programming.

@timdorr and #1739 (continued)

We also don’t want to use something like deep-freeze by default because it also has performance penalties and impacts correctness (we can’t just enable it in DEV and disable in PROD because the behavior would be different).

Out of curiosity, does anyone have examples of the correctness penalties to freezing objects? This seems to work well enough with redux-freeze's users (npm reports 35,427 downloads in the last month), so I'm curious if these issues could be solved. I agree that behavior should be mostly close between dev and production though.

@jimbolla

But the way I'd like to see it solved is to have something like a redux flavor of create-react-app that configures your store with some recommended store enhancers, with a mutation-detector as one of them.

This would be a cool project, but I think there's a similar problem with the current state of using redux-freeze with Redux: many users don't know these checks are necessary because they're not enabled by default to teach them why they're important. It's a catch 22 in a way.

@tkvw

I think the existence of these middlewares should be mentioned more permanently in docs at least.

That would be great, I'd like to see this at least if there isn't a good way to enable them in Redux by default. I'd be happy to help after learning more about these middlewares.

@markerikson

This comment has been minimized.

Copy link
Contributor

@markerikson markerikson commented Feb 28, 2018

This goes back to my comments in #2295 , about wanting to provide a "starter preset" of packages that would provide a Redux-team-recommended simplified store setup. I've seen dozens of variations on the theme, as well as "CRA + Redux" pieces out there already, but we haven't "blessed" any of them.

The real issue here is that the Redux core comes completely unconfigured out of the box. We make no assumptions about what middleware you might add in, or even if you're going to use middleware at all. So, we can't just add a dev middleware out of the box, because nothing is configured out of the box.

@nickmccurdy

This comment has been minimized.

Copy link
Contributor Author

@nickmccurdy nickmccurdy commented Feb 28, 2018

That would be excellent. Having something built into Redux would be the best for discoverability and developers upgrading Redux in existing apps, but having a blessed configuration in the docs would still be a big improvement. I guess the trickiest part would be figuring out how to support this nicely in CRA without requiring CRA, maybe it should be a library wrapping the store (like what Koala does for Koa apps).

How does Redux implement the existing checks that are enabled by default? Are they all based around internal actions rather than bundled middleware?

@markerikson

This comment has been minimized.

Copy link
Contributor

@markerikson markerikson commented Feb 28, 2018

Yeah, combineReducers and createStore both have actual internal checks for behavior (reducers not returning undefined, plain object actions, etc).

I'd say there's two or three things we could do here:

  • Have a docs section that demonstrates our specific recommended approach for setting up a Redux store from scratch, including HMR . (This is basically what I show in my post Practical Redux, Part 3: Project Setup .)
  • Create an official package that supplies a createReduxStore() function with good default settings out of the box, as well as possibly including additional pieces (per the ideas that I described in #2295 )
  • Create a CRA template-ish thing that creates a project with those settings by default

This also ties into my desire to better provide "next steps" after the basic tutorial process, as described in #2626 , and maybe even rearrange some of the docs structure to better emphasize some of these ideas.

I unfortunately don't have time to tackle any of this myself right now, but I'd be happy to provide direction and work with you (or anyone else who's interested in helping).

@timdorr

This comment has been minimized.

Copy link
Member

@timdorr timdorr commented Mar 1, 2018

Opened up #2859 for that discussion.

@nickmccurdy

This comment has been minimized.

Copy link
Contributor Author

@nickmccurdy nickmccurdy commented Mar 1, 2018

@markerikson Definitely leaning toward the second option personally, easier development experience while clashing with other boilerplates like CRA less. I'll continue discussion in #2859

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants
You can’t perform that action at this time.