You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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.
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.tsxit('should not call addEventListener on each render',async()=>{constcontainer=render(<SelectPaneltitle="title">child</SelectPanel>)
constdialogEl=container.getByRole('dialog',{hidden: true})constaddEventListenerSpy=jest.spyOn(dialogEl,'addEventListener')constremoveEventListenerSpy=jest.spyOn(dialogEl,'removeEventListener')container.rerender(<SelectPaneltitle="title">child</SelectPanel>)
expect(addEventListenerSpy).not.toHaveBeenCalled()expect(removeEventListenerSpy).not.toHaveBeenCalled()})
Version
v36.17.0
Browser
No response
The text was updated successfully, but these errors were encountered:
Description
SelectPanel2.SelectPanel
will calldialogEl.removeEventListener
anddialogEl.addEventListener
on every render, because the call toReact.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#L195The React docs describe this behavior here: https://react.dev/reference/react/useEffect#parameters
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.
Version
v36.17.0
Browser
No response
The text was updated successfully, but these errors were encountered: