SelectPanel2 Optimization: Do not add and remove keydown listener for escape key on every render

[bwittenberg]

Description

SelectPanel2.SelectPanel will call dialogEl.removeEventListener and dialogEl.addEventListener on every render, because the call to React.useEffect is not passed an array of dependencies at this callsite: https://github.com/primer/react/blob/main/packages/react/src/drafts/SelectPanel2/SelectPanel.tsx#L195

The React docs describe this behavior here: https://react.dev/reference/react/useEffect#parameters

If you omit this argument, your Effect will re-run after every re-render of the component.

I suspect the dependency array was an accidental omission.

Why am I filing this bug/optimization issue?

I was interested in making a contribution to this repository, and I wanted to start with a small, low impact change. Select components are interesting to me, so I began reviewing the component to look for ways to help, and I came across this as a potential optimization. It's a nitpick, and may not have any real impact on user performance, so I understand if the maintainers want to close without addressing.

Steps to reproduce

The following test case will fail for me when I added to https://github.com/primer/react/blob/main/packages/react/src/drafts/SelectPanel2/SelectPanel.test.tsx.

Note: This is a fairly "white box" test and relies on implementation details. I find that these kind of tests are useful to check performance optimizations, but the tradeoff is that the tests are more brittle.

// packages/react/src/drafts/SelectPanel2/SelectPanel.test.tsx
  it('should not call addEventListener on each render', async () => {
    const container = render(<SelectPanel title="title">child</SelectPanel>)
    const dialogEl = container.getByRole('dialog', {hidden: true})
    const addEventListenerSpy = jest.spyOn(dialogEl, 'addEventListener')
    const removeEventListenerSpy = jest.spyOn(dialogEl, 'removeEventListener')

    container.rerender(<SelectPanel title="title">child</SelectPanel>)
    expect(addEventListenerSpy).not.toHaveBeenCalled()
    expect(removeEventListenerSpy).not.toHaveBeenCalled()
  })

Version

v36.17.0

Browser

No response

[bwittenberg]

If the maintainers are interested, I fixed this issue locally. When I tried the push the branch, permission was denied.

I'm not sure if you are open to contributions or not, but if you are, I'd be happy to open a PR!