-
Notifications
You must be signed in to change notification settings - Fork 25
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
fix: remove mutations of the redux state in reducers #2487
Conversation
paths: [...attributionData.resourcesWithAttributedChildren.paths], | ||
pathsToIndices: { | ||
...attributionData.resourcesWithAttributedChildren.pathsToIndices, | ||
}, | ||
attributedChildren: { | ||
...attributionData.resourcesWithAttributedChildren.attributedChildren, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is called in several places and not always is every one of these parts mutated. To improve performance, we could just make a shallow copy here and then make copies of the respective parts, where they are mutated.
If we decide to do that, I think we should generally establish that pattern and also fix it at other places, though.
@@ -19,7 +19,7 @@ export function createAppStore() { | |||
// TECH DEBT: we should not be putting sets into the store | |||
// https://redux.js.org/style-guide/#do-not-put-non-serializable-values-in-state-or-actions | |||
serializableCheck: false, | |||
// TECH DEBT: we are mutating the redux state inside reducers | |||
// We set this to false because it significantly reduces performance in development mode. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Signed-off-by: Anton Bauhofer <anton.bauhofer@tngtech.com>
9b5a4f2
to
f7396c1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice. Thank you @antonbauhofer! 👍
I did a performance test with a large OpossumUI file and the performance does not seem to be significantly impacted by these changes. Also, I loosely checked that no other state mutations, not covered by tests, are present. |
Summary of changes
This fixes some of our reducers, so that they don't mutate the state.
Context and reason for change
Before this change, there were a few places in our code, where reducers were mutating the state. This is strongly discouraged according to the Redux guide.
How can the changes be tested
yarn test:unit
Fixes #2412