Skip to content
This repository was archived by the owner on Sep 30, 2025. It is now read-only.

[KeypressListener] Prevent listener attaching/detaching on rerender #4173

Merged
merged 10 commits into from
Jun 4, 2021
Merged

[KeypressListener] Prevent listener attaching/detaching on rerender #4173

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
508b554
bump to react-hooks 1.13.0, improve keypress perf
emma-boardman May 6, 2021
7378f97
add unreleased
emma-boardman May 6, 2021
58c2b58
remove handler dependency
emma-boardman May 6, 2021
db35209
remove additional yarn.lock ref
emma-boardman May 6, 2021
86b1a37
fix unreleased typo, update storybook rules
emma-boardman May 6, 2021
1d548a8
add local hook, useRef for handler and keyCode
emma-boardman May 11, 2021
7855b28
update unreleased
emma-boardman May 11, 2021
95afadd
improve unreleased, adjust keypress function order
emma-boardman May 11, 2021
1263948
restore original yarn.lock for react-hooks
emma-boardman May 12, 2021
90d0658
correct unreleased after rebase
emma-boardman Jun 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions UNRELEASED.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ Use [the changelog guidelines](https://git.io/polaris-changelog-guidelines) to f

### Enhancements

- Prevented `KeypressListener` attaching/detaching on every render ([#4173](https://github.com/Shopify/polaris-react/pull/4173))

### Bug fixes

### Documentation
Expand Down
6 changes: 2 additions & 4 deletions src/components/Autocomplete/components/ComboBox/ComboBox.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, {useState, useEffect, useLayoutEffect, useCallback} from 'react';
import React, {useState, useEffect, useCallback} from 'react';

import {useUniqueId} from '../../../../utilities/unique-id';
import {useToggle} from '../../../../utilities/use-toggle';
Expand All @@ -8,7 +8,7 @@ import {Popover, PopoverProps} from '../../../Popover';
import {ActionListItemDescriptor, Key} from '../../../../types';
import {KeypressListener} from '../../../KeypressListener';
import {EventListener} from '../../../EventListener';
import {isServer} from '../../../../utilities/target';
import {useIsomorphicLayoutEffect} from '../../../../utilities/use-isomorphic-layout-effect';

import {ComboBoxContext} from './context';
import styles from './ComboBox.scss';
Expand Down Expand Up @@ -71,8 +71,6 @@ export function ComboBox({
setFalse: forcePopoverActiveFalse,
} = useToggle(false);

const useIsomorphicLayoutEffect = isServer ? useEffect : useLayoutEffect;

const id = useUniqueId('ComboBox', idProp);

const getActionsWithIds = useCallback(
Expand Down
16 changes: 12 additions & 4 deletions src/components/KeypressListener/KeypressListener.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import {useEffect} from 'react';
import {useCallback, useEffect, useRef} from 'react';

import {useIsomorphicLayoutEffect} from '../../utilities/use-isomorphic-layout-effect';
import type {Key} from '../../types';

export interface KeypressListenerProps {
Expand All @@ -15,18 +16,25 @@ export function KeypressListener({
handler,
keyEvent = 'keyup',
}: KeypressListenerProps) {
const handleKeyEvent = (event: KeyboardEvent) => {
const tracked = useRef({handler, keyCode});

useIsomorphicLayoutEffect(() => {
Copy link
Member

@BPScott BPScott May 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]);

Copy link
Member

@clauderic clauderic May 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

tracked.current = {handler, keyCode};
}, [handler, keyCode]);

const handleKeyEvent = useCallback((event: KeyboardEvent) => {
const {handler, keyCode} = tracked.current;
if (event.keyCode === keyCode) {
handler(event);
}
};
}, []);

useEffect(() => {
document.addEventListener(keyEvent, handleKeyEvent);
return () => {
document.removeEventListener(keyEvent, handleKeyEvent);
};
});
}, [keyEvent, handleKeyEvent]);

return null;
}
5 changes: 5 additions & 0 deletions src/utilities/use-isomorphic-layout-effect.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import {useEffect, useLayoutEffect} from 'react';

import {isServer} from './target';

export const useIsomorphicLayoutEffect = isServer ? useEffect : useLayoutEffect;