You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "justinpark (via GitHub)" <gi...@apache.org> on 2023/04/28 19:33:18 UTC
[GitHub] [superset] justinpark opened a new pull request, #23871: refactor: useEffectEvent for efficient deps
justinpark opened a new pull request, #23871:
URL: https://github.com/apache/superset/pull/23871
### SUMMARY
We often to update(add) the useEffect dependencies to fix the dependency warnings (i.e. #22974)
This work has potential performance degrade issue.
For example, #22974 updated the following useEffect dependancies.
https://github.com/apache/superset/blob/097f47519433aa7892f1993a33afb3e0a12296a4/superset-frontend/src/SqlLab/components/SqlEditor/index.jsx#L382-L397
This means `setHeight` and window event allocation can be triggered anytime handleWindowResizeWithThrottle/onBeforeUnload or/and updated.
The problem is `onBeforeUnload` only has dependancies by useCallback
https://github.com/apache/superset/blob/097f47519433aa7892f1993a33afb3e0a12296a4/superset-frontend/src/SqlLab/components/SqlEditor/index.jsx#L365-L380
So whenever database is updated or latestQuery state updated, the above useEffect triggers the additional setHeight(which triggers re-rendering) and new event allocation (no memory leaked but unnecessary addEventListener/removeEventListener operation triggered)
This is common issue on react useEffect usage, so there's an official solution (useEffectEvent) is introduced.
For more detail, please check [the blog link /w video clip](https://morioh.com/p/994ce3afc1d3) or [official proposal document](https://github.com/reactjs/rfcs/blob/useevent/text/0000-useevent.md)
In addition to introduce the useEffectEvent, this commit also tune up L365-L380 to avoid the unnecessary setHeight triggered during database is updated.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A
### TESTING INSTRUCTIONS
Resize sqllab editor to verify the query panel height adjusted accordingly.
### 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
- [x] 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] justinpark merged pull request #23871: refactor: useEffectEvent for efficient deps
Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark merged PR #23871:
URL: https://github.com/apache/superset/pull/23871
--
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 #23871: refactor: useEffectEvent for efficient deps
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on code in PR #23871:
URL: https://github.com/apache/superset/pull/23871#discussion_r1181544067
##########
superset-frontend/src/SqlLab/components/SqlEditor/index.jsx:
##########
@@ -354,39 +355,28 @@ const SqlEditor = ({
return base;
}, [dispatch, queryEditor.sql, startQuery, stopQuery]);
- const handleWindowResize = useCallback(() => {
+ const handleWindowResize = useEffectEvent(() => {
setHeight(getSqlEditorHeight());
Review Comment:
```suggestion
setHeight(height);
```
##########
superset-frontend/src/SqlLab/components/SqlEditor/index.jsx:
##########
@@ -354,39 +355,28 @@ const SqlEditor = ({
return base;
}, [dispatch, queryEditor.sql, startQuery, stopQuery]);
- const handleWindowResize = useCallback(() => {
+ const handleWindowResize = useEffectEvent(() => {
Review Comment:
You should pass the `height` to the effect event. Check the **Notes** [here](https://react.dev/learn/separating-events-from-effects).
```suggestion
const handleWindowResize = useEffectEvent(height => {
```
--
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 #23871: refactor: useEffectEvent for efficient deps
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on code in PR #23871:
URL: https://github.com/apache/superset/pull/23871#discussion_r1182425923
##########
superset-frontend/src/SqlLab/components/SqlEditor/index.jsx:
##########
@@ -354,39 +355,28 @@ const SqlEditor = ({
return base;
}, [dispatch, queryEditor.sql, startQuery, stopQuery]);
- const handleWindowResize = useCallback(() => {
+ const handleWindowResize = useEffectEvent(() => {
setHeight(getSqlEditorHeight());
Review Comment:
Oh, that's right. I didn't notice it was a `useState` 👍🏼
--
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 #23871: refactor: useEffectEvent for efficient deps
Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark commented on code in PR #23871:
URL: https://github.com/apache/superset/pull/23871#discussion_r1181917806
##########
superset-frontend/src/SqlLab/components/SqlEditor/index.jsx:
##########
@@ -354,39 +355,28 @@ const SqlEditor = ({
return base;
}, [dispatch, queryEditor.sql, startQuery, stopQuery]);
- const handleWindowResize = useCallback(() => {
+ const handleWindowResize = useEffectEvent(() => {
Review Comment:
removed `handleWindowResize` since no longer needed
--
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 #23871: refactor: useEffectEvent for efficient deps
Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark commented on code in PR #23871:
URL: https://github.com/apache/superset/pull/23871#discussion_r1181912029
##########
superset-frontend/src/SqlLab/components/SqlEditor/index.jsx:
##########
@@ -354,39 +355,28 @@ const SqlEditor = ({
return base;
}, [dispatch, queryEditor.sql, startQuery, stopQuery]);
- const handleWindowResize = useCallback(() => {
+ const handleWindowResize = useEffectEvent(() => {
setHeight(getSqlEditorHeight());
Review Comment:
In this approach, this doesn't need to wrap with useEffectEvent since no references needed other than setHeight.
Let me clean up with setHeight then.
--
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 #23871: refactor: useEffectEvent for efficient deps
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #23871:
URL: https://github.com/apache/superset/pull/23871#issuecomment-1528294936
## [Codecov](https://codecov.io/gh/apache/superset/pull/23871?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 [#23871](https://codecov.io/gh/apache/superset/pull/23871?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (cd99108) into [master](https://codecov.io/gh/apache/superset/commit/594d3e0e0c4add06c1216490c63cdc6ac43e2f5a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (594d3e0) will **decrease** coverage by `0.01%`.
> The diff coverage is `55.00%`.
> :exclamation: Current head cd99108 differs from pull request most recent head b1077d7. Consider uploading reports for the commit b1077d7 to get more accurate results
```diff
@@ Coverage Diff @@
## master #23871 +/- ##
==========================================
- Coverage 68.11% 68.11% -0.01%
==========================================
Files 1938 1939 +1
Lines 74972 74982 +10
Branches 8141 8143 +2
==========================================
+ Hits 51067 51072 +5
- Misses 21826 21830 +4
- Partials 2079 2080 +1
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `54.47% <52.63%> (-0.01%)` | :arrow_down: |
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/23871?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [superset-frontend/src/hooks/useEffectEvent.ts](https://codecov.io/gh/apache/superset/pull/23871?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2hvb2tzL3VzZUVmZmVjdEV2ZW50LnRz) | `44.44% <44.44%> (ø)` | |
| [...frontend/src/SqlLab/components/SqlEditor/index.jsx](https://codecov.io/gh/apache/superset/pull/23871?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1NxbEVkaXRvci9pbmRleC5qc3g=) | `58.28% <60.00%> (ø)` | |
| [superset/db\_engine\_specs/dremio.py](https://codecov.io/gh/apache/superset/pull/23871?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2RyZW1pby5weQ==) | `95.23% <100.00%> (+0.23%)` | :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
[GitHub] [superset] michael-s-molina commented on pull request #23871: refactor: useEffectEvent for efficient deps
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #23871:
URL: https://github.com/apache/superset/pull/23871#issuecomment-1529673681
This will be indeed a nice addition to React. Looking forward to it.
--
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