Documentation/Changeling: Add ban of getState in reducers to the points

[9SMTM6]

*Changelog, gotta love autocorrect.

Bug

Something so essential should not just be included in the short text, but the supposedly more exhaustive list - preferably close to the top!

Also the ability to disable this ban would be great.

I completely understand why you would ban it, I was very annoyed when I saw the usage of this in the code of my project, but one does not choose the ... legacy code one gets by the people previously on the task, and to be honest while it's a bad behavior there's far worse elsewhere which I'd like to solve first before getting to this.

What is the current behavior?

That information is just in the summary in text form.

What is the expected behavior?

It's in the list.

Which versions of Redux, and which browser and OS are affected by this issue? Did this work in previous versions of Redux?
4.0+

[timdorr]

If you want to submit a PR to add this, you can. But using getState isn't very likely when state is provided as the first argument to your reducer function.

[9SMTM6]

Well, that doesn't help if the state you're referencing is outside of the state of the reducer. You could of course merge the reducers, but when you have such a gigantic network of reducers like my project (some madmen moved almost everything into redux, including lots of procedural variables) then merging reducers gets pretty complicated when they're not both in the same superreducer, additionally to leading to gigant reducers (there's already a number of 600er reducers, I don't need even longer reducers).

On the whole there's a lot of problems with the way some (or a lot of) legacy code is build, like also a complete lack of understanding of TypeScript, but I have to say some of the limitations of redux aren't making it easier.

Vuex has computed properties, making having a single source of truth much easier, it has method-style-getters making the modelling and embracing of state-dependant functions which can be reused elsewhere an ease - without bloating the HTML and JSX with a shitton of HOC - , with mutations (=redux actions) and it's - vue - actions it has a built-in principle to model the composition of potentially asynchronous 'actions' that makes at least redux-thunk, which my project uses, look old, you DONT have the problem of how to access outer state without breaking your pattern because their modules are more sensible, the documentation embraces helpful modern syntax thats easy to polyfill, like the spread syntax, and it DOES NOT facilitate a complete fragmentation of management of the same piece of state over multiple files, like lots of redux documentation does.

Do you have any idea how many files you have to touch sometimes to do anything in that mess I have to work with? Between redux, redux-thunk, TypeScript being an afterthought that they sometimes even implemented using freaking d.ts files with interfaces, that are meant for freaking libraries and completely break any interlinking from the IDE because they are redeclared at every corner and type inference is never used, redux-reacts 'container' adding some more bloat... Honestly if you look at all of that, and all the 'good ideas' people throw around that add some more fragmentation, I can't completely fault them for the mess that is the legacy code.

But I can tell you, it certainly doesn't help me getting that stuff straight if everyone wants to play good dictator and cuts one off of the modern versions of the packages because they arbitrarily add bans that are NOT listed in the checklist and instead hidden in the text. Tell you it feels great if after you spend a lot of time getting rid of all the compilation errors you realize upon testing that someone added a ban to a 'feature' frequently used in the code of your project.

I guess I should have read the text too, but not listing that ban in the list is actually pretty comparable to the thing you banned, you're hiding a sideeffect in the text that you'd expect in the list of everything, just like you'd expect all dependencies to be listed in the parameters of a function and not be recieved by store.getState() in the inside.

[markerikson]

@9SMTM6 : uh.... that was a long rant, and I'm not quite sure what the point is.

I get that you're frustrated, and I'm sorry to hear that.

That said, calling getState() in a reducer has always been wrong. You should never have been doing that. Reducers have always been supposed to be pure functions - just parameters in, result out.

I'll note that this change was listed in the release notes for v4.0.0:

https://github.com/reduxjs/redux/releases/tag/v4.0.0
#1569

If it's not currently mentioned in the docs, then yeah, we'd definitely accept a PR to add that mention.

[9SMTM6]

It's not listed in "Changes"

In fact the flow goes like this:

There's a paragraph comparing the changes to react 15 to 16, than there's another longer paragraph I sort of skipped over mostly (as I learnt later that was a mistake), mostly just reading the last sentence that "the full changes are listed below". Than there's a list. That list mentions a bunch of stuff, including that you'll throw errors when calling getState while dispatching actions and a bunch of TypeScript typing updates.

What's not mentioned in that "[list of] full changes" is that you throw if calling getState from the reducer. The ONLY place I found that mentioned is in the 2nd paragraph of the "summary". Where I found it AFTER I upgraded the huge project I'm working on, which wasn't all that easy actually as i.e. something broke the extension of the 'Dispatch' type by redux-thunk, leading to type errors and thus me needing to create a custom type and using that everywhere I used to just use 'Dispatch' as type.

So I put in all the work, the app compiles, then I try to test the app and realize that there's a ban on the usage of getState in reducers. Which btw throws nicely NOT while compiling but in the runtime, making said bans easy to slip past you if you only make some tests.

And yeah, I completely understand that you want to get rid of usage of getState in reducers but

• That's not my crap codebase I have to work with, and there's worse issues that both should be addressed first and are easier to fix

• If you allowed something in in the past, and replacing usage of that antipattern with something following the official pattern is a pain, then IMAO a ban on that 'feature' - if not essential for further development of the package - should be opt-out.

• Even if you HAD a opt out, not listing that change in the "full changes" is a no go, and essentially a worse antipattern than the very thing you just banned.

• Honestly there isn't really a good way in your pattern to do A LOT of stuff, including accessing the state of other reducers, when there would be an easy way to do that, just by adding an optional 3rd parameter to the reducers with the global state. If you don't use the 3rd parameter it's a classical reducer, but at the same times it allows you more freedom without having to revert to painful methods like duplicating state or merging reducers (said reducers sometimes being at completely different places in the reducer-tree). You should really take a look at some of the stuff Vuex (flux type state management for Vue) does, it's far superior to redux IMAO, maybe you could include at least SOME of their tricks in your pattern.

I guess I'll add a PR later when I'm not on mobile, at work or sleeping, but I honestly wonder how that slipped past.

It's especially annoying too that when you search the issue nearly all the discussions are referencing the 'redux-devtools-extension' as cause which apparently had the same ban (and got rid of it again?).

[9SMTM6]

I dont think I can make a pull request to edit releases on github.

Recommended change:

Exchange:

* Throw if getState, subscribe, or unsubscribe called while dispatching

for

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

@timdorr I'd appreaciate if youd fix that - if possible -, making the list of 'full changes' at least somewhat more complete.

[timdorr]

It literally says that at the top of the release message

The major changes (#1342) are around our TypeScript definitions, bundled CommonJS and ES builds, throwing if you subscribe or getState from a reducer, and a bunch of other smaller things. The full changes are listed below.