suddjian commented on a change in pull request #13220:
URL: https://github.com/apache/superset/pull/13220#discussion_r578815464
##########
File path: superset-frontend/src/explore/components/ExploreViewContainer.jsx
##########
@@ -146,11 +147,11 @@ const getWindowSize = () => ({
function useWindowSize({ delayMs = 250 } = {}) {
const [size, setSize] = useState(getWindowSize());
- useEffect(() => {
+ useComponentDidMount(() => {
const onWindowResize = debounce(() => setSize(getWindowSize()), delayMs);
window.addEventListener('resize', onWindowResize);
return () => window.removeEventListener('resize', onWindowResize);
- }, []);
+ });
Review comment:
I am wary of `useComponentDidMount` making it easier to ignore
dependencies. Hooks are a new style of business logic organization that focuses
on data flow rather than component lifecycles. Emulating the old component
lifecycle focused design and silencing lint warnings is a step backwards in my
view.
In this particular case, for example, rather than switching to
`useComponentDidMount`, it would probably be more correct here to add `delayMs`
to the dependencies array.
I do think there could be a place for `useComponentDidMount`, but only when
the logic in question specifically depends on only running at component mount.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
