suddjian commented on a change in pull request #13220:
URL: https://github.com/apache/superset/pull/13220#discussion_r579482405
##########
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 think removing the unrelated changes would be best. I'm okay with
having a `useComponentDidMount` hook around, as long as it is only used with
code that is specifically dependent on that life cycle event, and isn't just
using it to avoid lint warnings. A good example of this is the
LOG_ACTIONS_MOUNT_EXPLORER event, which is concerned specifically the explorer
container being mounted. I expect legit cases of `useComponentDidMount` to be
very rare.
A side note on the tech debt in `ExploreViewContainer`:
A lot of the warnings here are left over from when I converted it from a
class to a function component. I didn't have time to buff out all the edges
there. I'm confident that better organization of state and business logic in
this component would simplify the code and get rid of the warnings. For
example, the `addHistory` function is called from multiple places, because in
the class component we had to handle changes in that imperative way. But I
suspect this functionality could instead be placed in a `useEffect` hook that
sets the url based on state changes, which would be more declarative and robust.
That is what I was getting at when I said hooks are a new state management
pattern that isn't based on component lifecycles. KCD discusses this in the
post that @ktmud linked to as well.
----------------------------------------------------------------
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]
