You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "michael-s-molina (via GitHub)" <gi...@apache.org> on 2023/04/10 17:28:21 UTC
[GitHub] [superset] michael-s-molina opened a new pull request, #23637: fix: Removes Redux state mutations - iteration 3
michael-s-molina opened a new pull request, #23637:
URL: https://github.com/apache/superset/pull/23637
### SUMMARY
This iteration removes Redux state mutations from the codebase to allow us to merge https://github.com/apache/superset/pull/23460 without disrupting the development workflow.
### TESTING INSTRUCTIONS
1 - Pull changes from https://github.com/apache/superset/pull/23460 to enable the [immutabilityMiddleware](https://redux-toolkit.js.org/api/immutabilityMiddleware)
2 - Add or remove a native filter
3 - You shouldn't see any errors related to state mutation
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina merged pull request #23637: fix: Removes Redux state mutations - iteration 3
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina merged PR #23637:
URL: https://github.com/apache/superset/pull/23637
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] justinpark commented on a diff in pull request #23637: fix: Removes Redux state mutations - iteration 3
Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark commented on code in PR #23637:
URL: https://github.com/apache/superset/pull/23637#discussion_r1162060785
##########
superset-frontend/src/components/Chart/ChartRenderer.jsx:
##########
@@ -250,6 +250,11 @@ class ChartRenderer extends React.Component {
postTransformProps,
} = this.props;
+ // TODO: queriesResponse comes from Redux store but it's being edited by
+ // the plugins, hence we need to clone it to avoid state mutation
+ // until we change the reducers to use Redux Toolkit with Immer
+ const mutableQueriesResponse = cloneDeep(queriesResponse);
Review Comment:
This might cause the performance regression because it will deeply clone the same json response each rendering time (even though no queriesResponse updates).
We should build the clone value only queriesResponse has been updated.
(Since it's legacy component so need to revisit to fix later but the easiest way) My suggestion is using a clone reference in the construction and update during `shouldComponentUpdate` checking.
```
constructor(props) {
...
this.mutableQueriesResponse = cloneDeep(this.props.queriesResponse);
}
shouldComponentUpdate(nextProps, nextState) {
....
this.hasQueryResponseChange =
nextProps.queriesResponse !== this.props.queriesResponse;
if (this.hasQueryResponseChange) { this.mutableQueriesResponse = cloneDeep(nextProps.queriesResponse); }
...
render() {
...
queriesData={this.mutableQueriesResponse}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on pull request #23637: fix: Removes Redux state mutations - iteration 3
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #23637:
URL: https://github.com/apache/superset/pull/23637#issuecomment-1502100771
@justinpark I tested all major workflows with the results of this and previous iterations and I was not able to generate immutability errors. There are many errors related to non-serializable objects being stored in Redux like BigNumbers and functions but these only generate warnings and don't block your PR. We can fix them in follow-ups.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on a diff in pull request #23637: fix: Removes Redux state mutations - iteration 3
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on code in PR #23637:
URL: https://github.com/apache/superset/pull/23637#discussion_r1162064152
##########
superset-frontend/src/components/Chart/ChartRenderer.jsx:
##########
@@ -250,6 +250,11 @@ class ChartRenderer extends React.Component {
postTransformProps,
} = this.props;
+ // TODO: queriesResponse comes from Redux store but it's being edited by
+ // the plugins, hence we need to clone it to avoid state mutation
+ // until we change the reducers to use Redux Toolkit with Immer
+ const mutableQueriesResponse = cloneDeep(queriesResponse);
Review Comment:
Good suggestion. Will do.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] commented on pull request #23637: fix: Removes Redux state mutations - iteration 3
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #23637:
URL: https://github.com/apache/superset/pull/23637#issuecomment-1502149445
## [Codecov](https://codecov.io/gh/apache/superset/pull/23637?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#23637](https://codecov.io/gh/apache/superset/pull/23637?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (fc71546) into [master](https://codecov.io/gh/apache/superset/commit/b613167636aae82170b24f697d79fcd70ef1ac56?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (b613167) will **increase** coverage by `0.00%`.
> The diff coverage is `100.00%`.
> :exclamation: Current head fc71546 differs from pull request most recent head 3a8aaa7. Consider uploading reports for the commit 3a8aaa7 to get more accurate results
```diff
@@ Coverage Diff @@
## master #23637 +/- ##
=======================================
Coverage 67.71% 67.72%
=======================================
Files 1918 1918
Lines 74157 74158 +1
Branches 8053 8053
=======================================
+ Hits 50219 50220 +1
Misses 21885 21885
Partials 2053 2053
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `53.90% <100.00%> (+<0.01%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/23637?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...et-frontend/src/components/Chart/ChartRenderer.jsx](https://codecov.io/gh/apache/superset/pull/23637?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQ2hhcnQvQ2hhcnRSZW5kZXJlci5qc3g=) | `51.85% <100.00%> (+0.60%)` | :arrow_up: |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org