Skip to content
This repository has been archived by the owner on Jan 20, 2022. It is now read-only.

Fix Popover component not being able to render >800 times without taking a dump on the main thread #357

Merged
merged 1 commit into from
Jun 11, 2021

Conversation

brainkim
Copy link
Contributor

@brainkim brainkim commented Jun 11, 2021

📦 Published PR as canary version: 9.1.1-canary.357.9555.0

✨ Test out this PR locally via:

npm install @apollo/space-kit@9.1.1-canary.357.9555.0
# or 
yarn add @apollo/space-kit@9.1.1-canary.357.9555.0

@jgzuke jgzuke added the minor Increment the minor version when merged label Jun 11, 2021
Copy link
Member

@cheapsteak cheapsteak left a comment

Choose a reason for hiding this comment

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

Sweet, and thank you!
I'm surprised that just memoizing modifiers is enough, were the props like content and children red herring?

@cheapsteak
Copy link
Member

cheapsteak commented Jun 11, 2021

Should we do similarly for AbstractTooltip?

@cheapsteak
Copy link
Member

Ah just read the comment more carefully
Sounds like this only happens when the array of modifiers includes callbacks (which is just here)
Great find!

@brainkim
Copy link
Contributor Author

brainkim commented Jun 11, 2021

@cheapsteak I was thinking about this in the shower. Before we merge, one alternative is to define the callbacks at the module scope, so we don’t have to use useMemo(). The thing is, the problem would still exist if the fallbackPlacements variable changed, though it would occur at a much slower pace.

@brainkim brainkim merged commit 887a569 into main Jun 11, 2021
@brainkim brainkim deleted the brian/popover-leak-fix branch June 11, 2021 15:48
@apollo-bot2
Copy link
Collaborator

🚀 PR was released in v9.2.0 🚀

@apollo-bot2 apollo-bot2 added the released This issue/pull request has been released. label Jun 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
minor Increment the minor version when merged released This issue/pull request has been released.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants