[KeypressListener] Prevent listener attaching/detaching on rerender #4173
Conversation
|
Storybook error:
Investigation notes:
Resolution attempt #1: Try bumping polaris storybook-react to 1.13.0
Error message |
.storybook/main.js
Outdated
| config.module.rules.push({ | ||
| test: /\.mjs$/, | ||
| include: /node_modules/, | ||
| type: 'javascript/auto', | ||
| }); |
There was a problem hiding this comment.
Could you leave context on the PR as to how this relates to the change you made / why it was required? 🙏
There was a problem hiding this comment.
This corrected the storybook error documented here: #4173 (comment)
@kaelig are you able to elaborate on why we need to add this?
There was a problem hiding this comment.
I'm actually not sure this is the best way to fix this, but when I looked into ways to get you unblocked, this happened to work! I'd love to look deeper into the why and how this works, I'm but I'm short on time right now :(
There was a problem hiding this comment.
This will work for within polaris's storybook but it means that every consuming app needs to add this to their webpack config too to include polaris, which is not acceptable. Partners using create-react-app / next.js / anything that isn't sewing-kit will be impacted and that area of effect is way too high.
This will be a breaking change for a several consumers, please don't do it.
This is a sign that react-hooks isn't shipping valid strict esm code. That should be fixed at source. The good news is I've been working on changing quilt's build system to rollup, which I i believe might help with this, if you want to sit tight until that work is ready - https://github.com/Shopify/sewing-kit-next/pull/147 / https://github.com/Shopify/sewing-kit-next/issues/137.
.storybook/main.js
Outdated
| config.module.rules.push({ | ||
| test: /\.mjs$/, | ||
| include: /node_modules/, | ||
| type: 'javascript/auto', | ||
| }); |
There was a problem hiding this comment.
This will work for within polaris's storybook but it means that every consuming app needs to add this to their webpack config too to include polaris, which is not acceptable. Partners using create-react-app / next.js / anything that isn't sewing-kit will be impacted and that area of effect is way too high.
This will be a breaking change for a several consumers, please don't do it.
This is a sign that react-hooks isn't shipping valid strict esm code. That should be fixed at source. The good news is I've been working on changing quilt's build system to rollup, which I i believe might help with this, if you want to sit tight until that work is ready - https://github.com/Shopify/sewing-kit-next/pull/147 / https://github.com/Shopify/sewing-kit-next/issues/137.
|
Alternatively you could avoid the react-hooks dependency by recreating the hook as it is trivial (this is probably nicer than adding a new dependency). Turns out we do this already in Autocomplete's combobox: https://github.com/Shopify/polaris-react/blob/main/src/components/Autocomplete/components/ComboBox/ComboBox.tsx#L74 The source for the hook (to compare against what Autocomplete does; https://github.com/Shopify/quilt/blob/main/packages/react-hooks/src/hooks/isomorphic-layout-effect.ts polaris-react store its own hooks in the src/utilities folder (e.g. |
| [keyCode], | ||
| ); | ||
|
|
||
| useIsomorphicLayoutEffect(() => { |
There was a problem hiding this comment.
I don't think I understand the need for this here anyway? I would have expected that the memoisation of handleKeyEvent and specifying that as a dependency of the useEffect would have been enough to stop the removal/re-adding anyway? - as the handleKeyEvent function would no longer be recreated on every render of the component, and thus no longer trigger the useEffect on every render
(written but not tested)
const handleKeyEvent = useCallback((event: KeyboardEvent) => {
if (event.keyCode === keyCode) {
handler(event);
}
}), [handler, keyCode];
useEffect(() => {
document.addEventListener(keyEvent, handleKeyEvent);
return () => {
document.removeEventListener(keyEvent, handleKeyEvent);
};
});
}, [keyEvent, handleKeyEvent]);There was a problem hiding this comment.
@BPScott the snippet you have above delegates the responsibility of ensuring that the handler prop is memoized to the consumer of KeypressListener, otherwise the useEffect callback will run on every single render if handler is not memoized (since it is a dependency of handleKeyEvent, which is a dependency of the effect).
Components such as KeypressListener shouldn't assume that their consumers will always remember to memoize the handlers they pass to them, and should optimize for their consumers instead when it makes sense to do so. In this case it's fairly low overhead for KeypressListener to do this for its consumers and will drastically reduce the number of times listeners are detached/re-attached
The pattern introduced in this PR ensures that listeners aren't attached and re-attached on every single render regardless of whether the consumer remembers to memoize their handlers
emma-boardman
force-pushed
the
event-listeners-improve-performance
branch
2 times, most recently
from
b489e1c to
14aee29
Compare
emma-boardman
force-pushed
the
event-listeners-improve-performance
branch
from
94bf25b to
8d797bb
Compare
emma-boardman
force-pushed
the
event-listeners-improve-performance
branch
from
8d797bb to
f08c5e8
Compare
|
@BPScott I'm getting failures in the git workflow. Is that something to be worried about? It passes the PR CI builds 🤔
|
emma-boardman
force-pushed
the
event-listeners-improve-performance
branch
from
d363c6f to
22f0015
Compare
size-limit report
|
|
I created some performance flame charts, on main and on this branch. However, I'm not sure how indicative they are of the memoization impact. In both instances Does this help, or are there other ways I can measure the performance impact?
|
emma-boardman
force-pushed
the
event-listeners-improve-performance
branch
from
22f0015 to
1263948
Compare
|
Thank you for looking into this! The React devtools flamecharts only show components. You'll need to look at the Chrome DevTools flamecharts to see more granular data and you can also compare the number of resulting event listeners, possibly even looking at memory footprint impact when encountering a worst case scenario (very big listener function, lots of renders)... But that starts to get a bit overkill so you probably don't have to go that deep. Feel free to spend 30 more minutes at most on this — the PR looks good on principle and I thought flamecharts would have been a nice addition but they're not a requirement! |
I ran some experiments in Chrome devtools, and I couldn't see any evidence for this having reduced the number of resulting event listeners 😬 However, I'm probably doing it wrong 😅 I'm going to merge this and dig more into effective performance testing at a later date! Thank you for the review and for bringing my attention to flamecharts and profiling 😁 |
WHY are these changes introduced?
Prevents listeners being detached / reattached on every rerender
WHAT is this pull request doing?
useIsomorphicEffectis in v1.13.0handlerevent to a refhandleKeyEventinuseCallbackuseIsomorphicLayoutEffectto updatehandlerRefuseEffectdependency array