Add opt-out to the bans on getState, subscription Handling and dispatch in the reducers

[9SMTM6]

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.

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, 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]

I dont know how I would go about adding this in the documentation (and maybe someone else wants to do that?), also just to ensure: spread-syntax in the sourcecode will get polyfilled to be compatible with IE etc, right?

Also no idea why the pipeline is failing, locally the linter and the tests run fine.

[netlify]

Deploy preview for redux-docs ready!

Built with commit b5de7df

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

[9SMTM6]

So format:check is failing, but it doesnt really show a whole lot of errors.

Also if I run format locally it modifies a shitton of files, that would break the diff would it not? Or is it just something like different linebreaks, cause in the 'changed files' I looked at I couldnt find anything different.

[timdorr]

Sorry, but this isn't going to be accepted. If you need the old behavior, stick with 3.7.2. See #1568 for our reasoning and discussion.

[9SMTM6]

Oh yeah, theres a WHOLE lot of discussion on that topic. CURRENTLY this does not break anything so with this beeing opt-in, me riddling the code and documentation EVERYWHERE with the warning that this is an antipattern and might be removed in the future - including an console.error I just added - theres NO reason to just throw away the code I wrote and I'd like to be rejected by some more contributors than yourself - prefferably with other reasons than 'because we feel like banning that behavior' - before I accept this as closed.

@timdorr

[markerikson]

I'm sorry you're upset, but I'm with Tim on this one. As I said in the other issue, this has been an anti-pattern since the beginning, and your code shouldn't have been written that way to begin with. We're not going to change the Redux library behavior just because there's a codebase out there that abused that.

As Tim said, if you absolutely need to keep the current code running, stick with Redux 3.x, which doesn't yet have the error enforced.

As for the reducer logic aspect: there's been a bunch of suggested attempts to modify combineReducers to accept a third argument, but none of them have worked out. I'm open to ideas there, but it's unlikely that we're going to change that in the immediate near future. You're probably better off writing your own custom version of combineReducers() that does what you need and using that instead.

[9SMTM6]

@markerikson
It didnt even have a warning.

Could you please go against my arguments in detail in the renewed merge request?

I write it there in more detail, but I fail to see how exactly my changes break anything, and the complete blind reject of a possibility to keep up to date without immediately changing the complete codebase escapes my understanding.

Speaking from my personal frustration, I have to manage a large frontend were I cant just tell the stakeholders 'hold up, I need to revamp the state management completely'. I'm a working student, I cant ask that of the large company the team of my company works for. But what I CAN do is updating the code in the areas I currently work on. By that process the usages of getState will fade out, and for other people the warnings will hopefully facilitate the same, and if it doesnt It'll at least extend their code to newer versions for as long as the continued support doesnt break other stuff.

I dont ask for new features to be catered to this. All I'm asking for is for time while I gradually Improve the application I work on, and I believe I can think for myself and decide for myself what Issue takes priority for me. I have now learned excessively that you guys hate the usage of this with passion, thus I'll focus a change harder, but theres only so much one can do, and I still think theres larger issues.

When you decide to fade out the permittance to disable the bans you'll still have the possibility to stick ANY other parameters into that object at the end, as long as you dont create name-conflicts, so I DONT SEE WHATS SO BAD ABOUT THIS.

[markerikson]

Calling getState in a reducer is wrong. We're not going to add an escape hatch for it, because A) that would only encourage the behavior, B) we'd have to support that API through the next major version at least, and C) this is not a common issue that needs to be solved. We've already given you a workaround: stick with the older version of Redux.

I understand that you are having issues in your own application, and I'm sorry that it's causing you problems, but we are not responsible for fixing issues in your codebase. It's your job to do due diligence when investigating upgrades to libraries. When a library has a new major version, it's very likely that there will be breaking changes, and you need to evaluate all of those before making the decision to actually upgrade. (Speaking as an application developer myself at work, I have to dig through changelogs all the time when evaluating library versions.)

[9SMTM6]

@markerikson

The line concerning that in THE LIST is NOT clear at all:

* Throw if getState or subscribe are called while dispatching

To know that that means in the dispatcher, you need to know the technical detail that the reducer gets called synchronousely IN the dispatch, and that contradicts the suggestion of the name 'dispatch' and is specifically in contrast to the way react handles state-changes.

So if you do nothing else, AT LEAST make that changelog clear by modifying that line as follows:

* Throw if getState or subscribe are called in the reducer/while dispatching

[markerikson]

Yes, the fact that dispatching synchronously calls the reducer logic is a core part of Redux design, and always has been. You may or may not know it, but that's how it works. It's basically:

function createStore(reducer) {
    var state;
    var listeners = []

    function getState() {
        return state
    }
    
    function subscribe(listener) {
        listeners.push(listener)
        return function unsubscribe() {
            var index = listeners.indexOf(listener)
            listeners.splice(index, 1)
        }
    }
    
    function dispatch(action) {
        state = reducer(state, action)
        listeners.forEach(listener => listener())
    }

    dispatch({})

    return { dispatch, subscribe, getState }
}

How the React-Redux bindings handle updates is a separate concern.

[9SMTM6]

@markerikson
I've literally just modified that code so now I'm perfectly aware of that.

And React-Redux uses reduxs dispatch as far as I have seen

Thunk handles way different, youre not always dispatching in the redux sense when youre dispatching a thunk-action, this could add some more confusion.

What I'm saying is that I wasnt aware of that before, and considering I would say that among my team I'm the best when it comes to coding in the frontend, I would think that lots of developers working with redux are not aware of that. And these people might get the wrong impression by that changelog, thus I'm asking for an adjustment to fix that issue.

So would you be so nice and get tim to do so?

[markerikson]

I went ahead and updated the release note line.

The thing about thunks is that they aren't actually reaching the real store.dispatch() - it's part of the middleware pipeline leading up to that.

I still don't understand your comment about:

that contradicts the suggestion of the name 'dispatch' and is specifically in contrast to the way react handles state-changes.

but tbh the discussion so far has not been terribly fruitful, and it suggests that you don't have a good understanding of how the actual dispatch mechanism works and how React-Redux interacts with the Redux store. You might want to read through my post Idiomatic Redux: The History and Implementation of React-Redux to get a better understanding there.

[9SMTM6]

Thank you very much for the line-change.

Your link is an interesting read I guess, I was wondering how you manage state-updates without re-rendering the entire application, though that its using memoization was pretty clear. But while state-propagation in react-redux is because of said memoization and because of other stuff like the top-down flow of updates you want to enforce, more complicated nowadays, dispatch, from what I see, works pretty much how I expected. I had something pretty much like the class in 'connect in a Nutshell' in mind when approximating connect.

What I mean with

Thunk handles way different, youre not always dispatching in the redux sense when youre dispatching a thunk-action, this could add some more confusion.

Is that when you dispatch a thunk-action it doesnt use the dispatch as you so nicely defined above. First it determines whether you dispatched an object or an function. if its an object it uses the classical dispatch. Else the function gets executed and the return of the function passed through like the action is in a classic dispatch. So when you use 'getStore' inside of a thunk-action that doesnt get blocked - unless redux-thunk explicitly blocks that itself - , because youre not in the dispatch as defined in createStore.

that contradicts the suggestion of the name 'dispatch' and is specifically in contrast to the way react handles state-changes.

What I mean there is: When I think of 'dispatch' I think of an event. And in JS, which is after all still the language we're using and thus is used as reference from my brain, well JS events, as far as I remember, are not executed synchronousely.

This contrasts with i.e. Vuex, which is way clearer in their designation 'commit' of an mutation - which is, as I'm sure you know, pretty much the combination of a redux action with the connected cases in the reducer. While when you 'dispatch' VUEXES 'actions' - which are pretty much what redux-thunk tries to model, they behave, exactly like I'd expect, potentially asynchronous.

With regards to react, well, when you use this.setState this also gets executed asynchronousely, and not even neccessarily in order.

Btw, when I continuousely highlight how much I prefer Vuex over Redux thats not a result of me coming from Vuex to Redux and trying to apply Vuex syntax on Redux, I have never written a real Vuex application, and some small tests were only done by me after I was working with redux for a long time already - its instead a 'I have been annoyed at the way things have been solved - at least in our app - with Redux, and now that I look at the documentation of Vuex they make it way better and make it look easy.'. I'm not sure youre making yourself a favour in trying to keep stuff like async-actions, dependent state and shared state between reducers out of this package. Like looking at Redux-Thunk vs Vues Actions, it MAY be somewhat comparable in code, though Redux-Thunk is more verbose, the problem arises in documentation and compatibility with the other little helpers.

In larger companies there will always be stuff like 'fullstack' developers trying to get some Frontend stuff done, looking at other code, seeing redux-thunk-actions and thinking 'ah, i need an action so thats what I was looking for', and then continuing to create a thunk-action which, to keep in line with the other thunk-actions returns a promise, but is in fact a completely synchronous action. Which then leads to ridiculous promise-chains that are not nedded at all. With Vues clear separetion between mutations and actions, with the differences made clear by different and fitting names and a documentation thats very much more on point than redux, that wont happen.

Sometime being opiniated isnt a bad thing. The thing is, the one thing you seem to be very opiniated about is removing getState from reducers. But removing these getState from the reducers is VERY hard at times, because I need to access global state. Guess what? Vues modules get a 3rd parameter thats the global state, and are in the end, while not enforcing an immutable approach (but VERY MUCH allowing it) still pure functions.

We also have a number of functions that use store.getState() outside of the components and are a big pain to me. Now to inject the store for the tests I could stuff them all in a factory function to pass the store, but then I have a shitton of factory-functions all over the place, and theyre not nice to look at. But I really need to reuse these functions in multiple places. The best way I could come up with to model these in the redux-ecosystem - as having functions in the state is an antipattern - is by packing them together by using connect with a mapState, mapDispatch and mergeProps and then adding that to every component I need to use these, pass these functions by argmuent a few levels if I have to. Vuex? https://vuex.vuejs.org/guide/getters.html#method-style-access. Super easy and in line with their derived state pattern.

Please, tell me how to do it better with redux.