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

useSelector code is now checking for current state changes which breaks current applications #1813

Closed
1 task done
suncodefactory opened this issue Sep 6, 2021 · 18 comments

Comments

@suncodefactory
Copy link

suncodefactory commented Sep 6, 2021

What version of React, ReactDOM/React Native, Redux, and React Redux are you using?

  • React:
  • ReactDOM/React Native:
  • Redux:
  • React Redux:

What is the current behavior?

in version 7.2.5 a new piece of code was added

image

This is just a reference check and it is causing a lot of useSelectors to not be called. Specially if you use the second parameter which is a callback to compare for equality.

Not everyone relies on the default equality comparer and not everyone replaces the entire state object if just one property changes for example. That would be overkill with complex stores and complex apps.

What is the expected behavior?

The expected behavior is that when the useSelector takes a second parameter with an equality comparer function to let the useSelector decide if the state has changed.

Which browser and OS are affected by this issue?

All browsers all OS

Did this work in previous versions of React Redux?

  • Yes
@markerikson
Copy link
Contributor

markerikson commented Sep 6, 2021

This is the same "bail out if the root state is unchanged" check that we've used in connect for years.

React-Redux has always expected that all store updates are done immutably:

https://blog.isquaredsoftware.com/2018/11/react-redux-history-implementation/#ui-updates-require-store-immutability

This means that every action that causes any update to any piece of state will result in a new root state object.

The second param to useSelector is specifically for deciding if the result of the selector has changed or not, but that's a separate question from whether the root state has changed.

If you have a specific use case that this is affecting, please provide a CodeSandbox or a repo that demonstrates the issue, including examples of how you are currently writing reducer and update logic.

If what you're doing involves actually mutating the current state, then from our perspective that is a bug in your app and you should not be doing that:

https://redux.js.org/style-guide/style-guide#do-not-mutate-state

@suncodefactory
Copy link
Author

suncodefactory commented Sep 6, 2021

This is the same "bail out if the root state is unchanged" check that we've used in connect for years.

React-Redux has always expected that all store updates are done immutably:

https://blog.isquaredsoftware.com/2018/11/react-redux-history-implementation/#ui-updates-require-store-immutability

This means that every action that causes any update to any piece of state will result in a new root state object.

The second param to useSelector is specifically for deciding if the result of the selector has changed or not, but that's a separate question from whether the root state has changed.

If you have a specific use case that this is affecting, please provide a CodeSandbox or a repo that demonstrates the issue, including examples of how you are currently writing reducer and update logic.

If what you're doing involves actually mutating the current state, then from our perspective that is a bug in your app and you should not be doing that:

https://redux.js.org/style-guide/style-guide#do-not-mutate-state

So basically this was working and as soon as I moved to version 7.2.5 it broke half the app.... because we always use the second parameter of the useSelector to decide if the state changed or not.

Any one who has worked with react apps that have thousands of components rendered in a single screen would know better than to always spread your state to a new object just because a single property changed for example.

We have stores that are many megabytes worth of json and trying to work that out immutably would mean.... that we would just have to use something other than react.

Also this change defeats the purpose of the useSelector's granularity power, because we might as well have just a useSelector that returns the complete state in all components. The correct behavior should be that the caller of the useSelector should be able to decide when the state changed based on the second parameter. Otherwise what is the use of the second parameter? Has the second prameter been deprecated?

Furthermore for apps that are "live" and are receiving thousands of changes and have really complex stores this change just basically means we need to find a new technology and move away from react-redux. We cannot create new instances of the state every time it changes because the browser would soon crash from the memory leak.

Our app handles thousands of changes on the screen and values are updating constantly on thousands of components rendered. Our app handles market analysis and needs to work "live".

@markerikson
Copy link
Contributor

markerikson commented Sep 6, 2021

@suncodefactory I don't think you read what I just said.

  • The second parameter of useSelector decides whether the return value of the selector has changed. This new check is for whether the root state has changed
  • It doesn't matter how many megabytes of JS objects you have in your store state, you're still supposed to be doing immutable updates
  • Proper immutable updates involve making copies of each level of nested data that needs to be updated. That always includes the root state
  • If you dispatch an action that results in an update to state.a.b.c, then your state.x.y.z should still be completely unchanged and reused with no copies, and it's therefore irrelevant how big the .z field might be

Any one who has worked with react apps that have thousands of components rendered in a single screen would know better than to always spread your state to a new object...
...
As for immutability that is a very big and opinionated assumption.

As I just said, React-Redux has always assumed immutable updates, period. And for that matter, React itself completely relies on immutable updates for useState and useReducer.. It's not like this is some kind of hidden secret.

So, I'll ask again: if you have specific concerns about this, please post a CodeSandbox or repo that shows what your application is doing. But, based on your description thus far, it sounds as if you are mutating the state in your application, and that is not how you are supposed to use Redux. Therefore, your app appears to be buggy.

@suncodefactory
Copy link
Author

If that was true that react-redux always depended on state instance id changing on every mutation than why was this not broken on version 7.2.4? As a matter of fact this was not broken on any previous version.

As for "buggy". I wish you could try to make thousands of changes an hour on the screen with stores that are 10 MB and use immutabilty on 6K components on a screen.... This is the only reason we use useSelectors because it gave us the ability to create this app without crashing the browser. I guess react and react-redux is not the right technology for this kind of app. No worries I will start looking for a way to migrate to a different framework perhaps to a "home brewed" designed for high traffic-high performance apps.

Thanks anyways

@markerikson
Copy link
Contributor

markerikson commented Sep 6, 2021

@suncodefactory I would like to try to offer more specific suggestions, but I need you to provide examples of what you're doing in your application. I can't help you if you can't or won't give me information on how your app is actually using React-Redux. Show me your reducers. Show me your useSelector calls. I'm asking you to give me the info I need to help you.

The reason this change was made in 7.2.5 was because useSelector was actually missing this piece of behavior that was always in connect, and that was causing selectors to run twice on mount when they didn't need to. So, this was both fixing an existing bug (selectors running more often than needed), and making useSelector behave the same way as connect always has.

@timdorr
Copy link
Member

timdorr commented Sep 6, 2021

Also this change defeats the purpose of the useSelector's granularity power, because we might as well have just a useSelector that returns the complete state in all components. The correct behavior should be that the caller of the useSelector should be able to decide when the state changed based on the second parameter. Otherwise what is the use of the second parameter? Has the second parameter been deprecated?

That is to compare the equality of the selected state, not the store itself. The store state will retain referential equality until you dispatch. Once you do, the new store state should be a new instance.

const oldState = store.getState()
store.dispatch(action)

oldState !== store.getState() // <- This must always be true!

Given that fact of how Redux works, your useSelector calls will always return the same values for the same store state. That's what this new optimization is doing (checking that we don't have a new store state), which results in less renders for most applications.

@timdorr
Copy link
Member

timdorr commented Sep 6, 2021

Simply put, if useSelector is returning two different results without any dispatch happening, you're using Redux wrong. If you need mutability, it sounds like you should be using a context instead.

This isn't a bug with the library.

@timdorr timdorr closed this as completed Sep 6, 2021
@markerikson
Copy link
Contributor

markerikson commented Sep 6, 2021

I will say that connect has had an areStatesEqual comparison option since v5. I could imagine us adding that to useSelector somehow, possibly as part of a new options object.

Alternately, if v7.2.4 works for you, you can always pin your app to use that. It's permanently available.

@markerikson
Copy link
Contributor

if useSelector is returning two different results without any dispatch happening

I'm reading it the other way around - that they are dispatching, but that the reducers are truly mutating the existing state as an attempted performance optimization.

But I can't know for sure until the OP actually provides some code examples.

@phryneas
Copy link
Member

phryneas commented Sep 6, 2021

Adding to that: having a few ten megabytes of data in your app and updating that only a few thousand times an hour should definitely not lead to a memory leak unless you have closures & variables that hold onto old references making it impossible for the garbage collector to remove those old instances from memory.

So, unless you have a bug in your application that is causing a memory leak, using Redux the intended way will definitely not be a problem.
I suggest you search for that bug instead of working around it in unsupported and undocumented ways.

@suncodefactory
Copy link
Author

Adding to that: having a few ten megabytes of data in your app and updating that only a few thousand times an hour should definitely not lead to a memory leak unless you have closures & variables that hold onto old references making it impossible for the garbage collector to remove those old instances from memory.

So, unless you have a bug in your application that is causing a memory leak, using Redux the intended way will definitely not be a problem.
I suggest you search for that bug instead of working around it in unsupported and undocumented ways.

You are assuming too much.... Few ten megabytes that get reinstantiated frequently will become gigabytes pretty quickly. Even if they get garbage collected then GC will soon become also a problem.

I am not sure what your experience is with this. But in my case we have reached a point where there is no memory leak and there is no further optimization we can do to the react rendering short of writing our own framework.

Our application has many thousands of components in the screen at all times and receive "live updates" that are quite complex.

If we just reinstantiate(immutability) the state on every mutation the browser will simply crash.

This is not some "shopping cart", This is an app is for market analysis and is very intensive.

We have to do every update and every render of every component with scalpel precision.

We have already been down the road of the "recommended" way of "doing things in react". What we found through a lot of profiling, instrumentation and measurements is that the "recommended way" is the worst possible way of doing react + react-redux.

Yeah it works for "tradional apps" because it guarantees to the programmer that their component will update with a simple reference check from a useSelector....

But for any serious app this is just not realistic. Yeah I know everyone believes it and its everywhere on the internet so "it must be true, the problem must be a bug".

But when you have to build an app like the one we build every single byte counts. Every mistake can cause a disaster.

So believe it or not I too know how to write code.

Plus this worked just fine forever until the last version of redux a few days ago. So it is breaking change... It is not a "feature" request.

@suncodefactory
Copy link
Author

suncodefactory commented Sep 13, 2021

if useSelector is returning two different results without any dispatch happening

I'm reading it the other way around - that they are dispatching, but that the reducers are truly mutating the existing state as an attempted performance optimization.

But I can't know for sure until the OP actually provides some code examples.

This is the exact situation. We are using reducers and dispatchers etc.....

Just that for our app creating instances of the state every single time a property changes is not an option.

I know everyone says "then it must be a bug".

But I have already pointed out what the problem is. Should be very easy to reproduce.

As matter of fact I actually pointed out the "guilty line of code" that was added to the useSelector file in redux a few days ago.

This was working forever and now its broken because the second parameter of the useSelector will only be called when the state is a new instance... which for us is already too late.

This means that the state has already changed. So in my opinion there is some contradiction in the concept of the useSelector's second parameter. Because if immutability is the "only signal" that the state has changed... then what is the purpose of the useSelector? is the use selector a simple react render suppressor? or is the useSelector's second parameter a true mechanism to allow a developer to decide when the state "has changed".

The only thing I am asking is for a mechanism to keep the same behavior that redux had previous to the addition of the one line of code added on version 7.2.5.

In my opinion the logic should be "perform the check if the second parameter is undefined or null". Because if the developer is assuming the responsibility for the equality check then it should be up to the developer to decide if the state has changed or not.

@phryneas
Copy link
Member

@suncodefactory Garbage collection can easily kick in a few times every second. We are talking about computers that are designed to do hundreds of thousand things every second.

Constructively: There might be one source of memory leak there though that you might have missed: the Redux Devtools actually keep references to old store values, so if you have those enabled I can imagine your browser crawling to a halt easily.

Generally though: you have based your application around a bug. A behaviour that was never part of any official API, never documented everywhere - and never really intended.

The only valid way of using Redux per the documentation is immutability. It is everywhere in the docs and it is even documented as one of the "three principles of Redux". This is not something to be waved away as "optional" and "doing things another way is fine".
Doing that is incredibly reckless and if you had asked anyone (via ticket, twitter or just in the Reactiflux chat) before, everyone would have told you "this is not intended, it is undocumented, it will break at any point".

Right now, useSelector is getting re-implemented to use React's new useSyncExternalStore hook and there is a good chance that even if this is rolled back, the new implementation would not work either: most of the implementation is now part of React, not react-redux any more, and will even be fundamentally different between React 16/17 and React 18.
So you can already see what you are getting into, you can take a look at the uSES shim (to be used with 16/17) here (facebook/react#22211) and the new useSelector implementation here (#1808).
It might start working with that again, it might become impossible - honestly, I don't know. But going that path forward, this is probably pretty much out of our hands. React also very heavily relies on the assumption of "immutable snapshots" from how I remember the latest state of it.

@suncodefactory
Copy link
Author

@suncodefactory Garbage collection can easily kick in a few times every second. We are talking about computers that are designed to do hundreds of thousand things every second.

Constructively: There might be one source of memory leak there though that you might have missed: the Redux Devtools actually keep references to old store values, so if you have those enabled I can imagine your browser crawling to a halt easily.

Generally though: you have based your application around a bug. A behaviour that was never part of any official API, never documented everywhere - and never really intended.

The only valid way of using Redux per the documentation is immutability. It is everywhere in the docs and it is even documented as one of the "three principles of Redux". This is not something to be waved away as "optional" and "doing things another way is fine".
Doing that is incredibly reckless and if you had asked anyone (via ticket, twitter or just in the Reactiflux chat) before, everyone would have told you "this is not intended, it is undocumented, it will break at any point".

Right now, useSelector is getting re-implemented to use React's new useSyncExternalStore hook and there is a good chance that even if this is rolled back, the new implementation would not work either: most of the implementation is now part of React, not react-redux any more, and will even be fundamentally different between React 16/17 and React 18.
So you can already see what you are getting into, you can take a look at the uSES shim (to be used with 16/17) here (facebook/react#22211) and the new useSelector implementation here (#1808).
It might start working with that again, it might become impossible - honestly, I don't know. But going that path forward, this is probably pretty much out of our hands. React also very heavily relies on the assumption of "immutable snapshots" from how I remember the latest state of it.

React DevTools are not installed on production. The users are not developers so they don't have any dev tools installed on their browsers. We control the browser version of all the users.

As for "your application was based on a bug" I dont think that just because something is not documented it automatically becomes a bug.

As for the change being really complex.... that is just not true.. It is a single line of code added to version 7.2.5 and all it needs it's to add an extra check to the if statement to make sure the second parameter of the use selector is undefined or null.

Basically if the developer is using his/her own equality check then useSelector should no longer make any assumptions on equality.

About garbage collection we already ran all kinds of profiling tools to see memory, garbage collection, calls, renders etc etc..

We were able to optimize the app to the maximum and took quite a while to get it right.

And you might be right. Maybe we just chose the wrong technology for this. Maybe react and react-redux is just not designed to withstand such usage.

Thanks for your reply. I had little hope anyways as I already knew that everyone was going come out with the "immutability" card.

For now we just decided to stick to version 7.2.4 until we switch to a framework tailored for our scenario.

Thanks everyone for your help.

Just a little disappointed that such a small change in react-redux means so much investment for others in terms of money and time.

@markerikson
Copy link
Contributor

markerikson commented Sep 13, 2021

@suncodefactory : Lenz's points are all entirely correct. As has been stated numerous times, Redux and React-Redux have always expected that all state updates are done immutably, and state mutations are a bug from our perspective. This is clearly stated all over our docs:

connect has had this exact same "has the root state changed?" check for years, and the fact that useSelector didn't have it until now was really an oversight that we should have corrected years ago.

You're also still misunderstanding what the equalityFn option for useSelector is meant for. The comparison function is only meant for comparing the results of the selector - not the root state itself!

I'm still open to the idea of adding an isRootStateEqual option of some kind, just because connect has also had that option for years. But, that's just going to be a bandaid and not work any "better" than 7.2.4, so in that sense you might as well just stick with 7.2.4.

As Lenz said, the other issue here is that immutability is going to become even more critical in React-Redux v8 and React 18, as we are changing the implementation of useSelector to defer to React's useSyncExternalStore hook for the real logic. I can guarantee that mutating your state will break UI updates with that implementation.

What you have right now "works", in a sense, but it's not at all what you're supposed to be doing. I'll be honest and say that it sounds like the kind of app you're building isn't the ideal use case for React and Redux, and you probably should look at other options. Perhaps you should consider something like Solid, or if you want to stick with React, look at Mobx instead of Redux.

@suncodefactory
Copy link
Author

@suncodefactory : Lenz's points are all entirely correct. As has been stated numerous times, Redux and React-Redux have always expected that all state updates are done immutably, and state mutations are a bug from our perspective. This is clearly stated all over our docs:

* https://redux.js.org/tutorials/essentials/part-1-overview-concepts#immutability

* https://redux.js.org/tutorials/essentials/part-2-app-structure#reducers-and-immutable-updates

* https://redux.js.org/faq/immutable-data

* https://blog.isquaredsoftware.com/2018/11/react-redux-history-implementation/#ui-updates-require-store-immutability

connect has had this exact same "has the root state changed?" check for years, and the fact that useSelector didn't have it until now was really an oversight that we should have corrected years ago.

You're also still misunderstanding what the equalityFn option for useSelector is meant for. The comparison function is only meant for comparing the results of the selector - not the root state itself!

I'm still open to the idea of adding an isRootStateEqual option of some kind, just because connect has also had that option for years. But, that's just going to be a bandaid and not work any "better" than 7.2.4, so in that sense you might as well just stick with 7.2.4.

As Lenz said, the other issue here is that immutability is going to become even more critical in React-Redux v8 and React 18, as we are changing the implementation of useSelector to defer to React's useSyncExternalStore hook for the real logic. I can guarantee that mutating your state will break UI updates with that implementation.

What you have right now "works", in a sense, but it's not at all what you're supposed to be doing. I'll be honest and say that it sounds like the kind of app you're building isn't the ideal use case for React and Redux, and you probably should look at other options. Perhaps you should consider something like Solid, or if you want to stick with React, look at Mobx instead of Redux.

I do agree with you. And we have decided to just write our own simple framework designed for the specific use case.

We are just going to avoid core dependencies on third party libraries to avoid the monetary cost of chasing trends.

Once again thanx for replying and putting your time into answering.

@phryneas
Copy link
Member

We are just going to avoid core dependencies on third party libraries to avoid the monetary cost of chasing trends.

Honestly, the lesson you should learn out of this should not be "don't use third party libraries", but "use third party libraries only for documented use cases, or be prepared to maintain your own fork (which is fine:tm:)".
Third party libraries (carefully chosen for your use case) will probably save you hundreds to thousands of hours of development time - all those potential bugs have already been experienced by others and are already fixed.

@suncodefactory
Copy link
Author

We are just going to avoid core dependencies on third party libraries to avoid the monetary cost of chasing trends.

Honestly, the lesson you should learn out of this should not be "don't use third party libraries", but "use third party libraries only for documented use cases, or be prepared to maintain your own fork (which is fine™️)".
Third party libraries (carefully chosen for your use case) will probably save you hundreds to thousands of hours of development time - all those potential bugs have already been experienced by others and are already fixed.

Not true in this case. Because when you choose a third party library you don't really know what the outcome is going to be after 2 years of developing an application.

For example all over the internet is preached like a religion the "high performance of react + react-redux". And there is no way to foresee that it wont work once your app is too big to change framework easily.

Specially because at the beginning it does work... but as you add more components, stores, reducers etc and your app grows in complexity it gets slower and slower. To the point where now a lot of time is being spent on trying to "fix it".

Until you finally realize the problem is the technology chosen for the app.

This is a gradual process. And claims about performance are abundant on the internet about react + react-redux.

And the whole "use only for documented cases" I am sorry to have to say this. But if I use react + react-redux only as documented our app would have been a dead project months ago. We were actually able to salvage the situation by exploring react and react-redux code and finding the weaknesses and work around them,

So no. Basically you are wrong. We did not start using react + react-redux in a "non documented case". We actually started perfectly following all the "instructions" and principles for immutability etc...

We started stripping away the pieces gradually over a long period of time just to make the app run at a decent performance.

And to be honest we never really saw the "high performance" that is preached everywhere on the internet.

But what you call "bugs" actually allowed us to run thousands of components with very complex object graphs as state.

I can guarantee that if we change our code to follow the "documented" way.... Our app would not be able to handle even half of the components and objects in the state.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants