-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Comments
This is the same "bail out if the root state is unchanged" check that we've used in React-Redux has always expected that all store updates are done immutably: 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 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". |
@suncodefactory I don't think you read what I just said.
As I just said, React-Redux has always assumed immutable updates, period. And for that matter, React itself completely relies on immutable updates for 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. |
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 |
@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 The reason this change was made in 7.2.5 was because |
That is to compare the equality of the selected state, not the store itself. The store state will retain referential equality until you const oldState = store.getState()
store.dispatch(action)
oldState !== store.getState() // <- This must always be true! Given that fact of how Redux works, your |
Simply put, if This isn't a bug with the library. |
I will say that Alternately, if v7.2.4 works for you, you can always pin your app to use that. It's permanently available. |
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. |
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. |
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. |
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. |
@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". Right now, |
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. |
@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:
You're also still misunderstanding what the I'm still open to the idea of adding an 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 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. |
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:)". |
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. |
What version of React, ReactDOM/React Native, Redux, and React Redux are you using?
What is the current behavior?
in version 7.2.5 a new piece of code was added
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?
The text was updated successfully, but these errors were encountered: