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

Add opt-out - with deprecation warnings - to the bans on getState, subscription Handling and dispatch in the reducers #3444

Closed
wants to merge 7 commits into from

Conversation

9SMTM6
Copy link

@9SMTM6 9SMTM6 commented Jun 7, 2019

Its not always easy to convert legacy code you may not even have written yourself, and as long as the continued acceptance of these antipatterns doesnt hurt other efforts I'd suggest the option for an opt-out of these bans, so that we dont force people to use older versions just because they feel like theres higher priorities than just completely revamping their applications state management right now.

This brings no breaking changes and the 'options' object may be expanded to allow for further configuration in the future. The documentation is riddled with warnings, and a console.error is set off every time a store gets created with one of these bans lifted. @timdorr this should COMPLETELY satisfy your apparent need to remove this behavior, as long as you dont provide a better reason I can accept or everyone else tells me they dont want to enable other people with legacy code to use the new features of Redux 4, just because these people think that using these functions in the reducer, while very bad, isnt the worst issue their app has currently and doesnt legitimize immediate complete workover of their potentially huge application.

This options object has been purposefully coded in a way that allows for expansion in the future, thus this does NOT prevent future expansions of arguments. I'd reccomend putting these in that options-object, as theyre unlikely to get used frequently and in that way - because of the non-sequential nature of the object-parameters - dont worsen the already bad situation currently where you have a headache with allowing different configurations without horrible syntax like passing in multiple undefined.

If this gets accepted and people are interested in that I might bring another merge request later which would allow a 3rd parameter in reducers which is passed the global state - thats also mentioned in the issue #1568 mentioned by @timdorr, the thing is THATS CURRENTLY INCOMPATIBLE WITH YOUR OWN COMBINEREDUCERS - allowing for in-pattern usage of the global state without having to revert to store.getState, which makes store-injection in testing very hard. This too would not break normal usage of redux, and when you use object destructuring in the argument to limit the scope of parameters you inject from the global state it also shouldnt lead to a to large state potentially influencing the reducer.

9SMTM6 and others added 7 commits June 7, 2019 17:23
Its a new last parameter to the createStore function, optional and filled with defaults.

Could be used for more stuff later.

Unfortunately including the enhancer in that object would be difficult as it would lead to difficulties differentiating between if the 2nd argument is the default state or the options object.
@netlify
Copy link

netlify bot commented Jun 7, 2019

Deploy preview for redux-docs ready!

Built with commit fdb00d0

https://deploy-preview-3444--redux-docs.netlify.com

@markerikson
Copy link
Contributor

Please stop re-submitting this.

@markerikson markerikson closed this Jun 7, 2019
@reduxjs reduxjs locked as off-topic and limited conversation to collaborators Jun 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants