-
-
Notifications
You must be signed in to change notification settings - Fork 15.3k
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
Comments
See #1739 (comment) |
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. |
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. |
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.
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)
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.
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.
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. |
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. |
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? |
Yeah, I'd say there's two or three things we could do here:
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). |
Opened up #2859 for that discussion. |
@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 |
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.
The text was updated successfully, but these errors were encountered: