Skip to content

fix: memoize chip color resolution#4964

Open
popsiclelmlm wants to merge 1 commit into
callstack:mainfrom
popsiclelmlm:perf/memoize-derived-colors
Open

fix: memoize chip color resolution#4964
popsiclelmlm wants to merge 1 commit into
callstack:mainfrom
popsiclelmlm:perf/memoize-derived-colors

Conversation

@popsiclelmlm
Copy link
Copy Markdown

Motivation

Chip resolves multiple theme-derived colors during render. The current main branch has already removed most of the color chains mentioned in issue #4946, but Chip still calls getChipColors every render even when its color inputs are unchanged.

This wraps the color resolution in React.useMemo so the derived color object is recomputed only when the relevant inputs change. There is no API or visual behavior change.

Related issue

Related to #4946.

Test plan

  • yarn test src/components/__tests__/Chip.test.tsx --runInBand
  • yarn lint-no-fix src/components/Chip/Chip.tsx
  • yarn typescript
  • Full test suite also ran during the pre-commit hook: 55 suites / 736 tests passed.

@callstack-bot
Copy link
Copy Markdown

Hey @popsiclelmlm, thank you for your pull request 🤗. The documentation from this branch can be viewed here.

@satya164
Copy link
Copy Markdown
Member

Thanks for the PR. Are you seeing any slowdowns or is this hypothetical?

Understanding it is critical to know whether rest of the code should be updated.

@popsiclelmlm
Copy link
Copy Markdown
Author

Thanks for asking. This isn't based on a benchmark or a user-reported slowdown. I noticed that Chip recalculates derived colors during render, so this PR is a preventative cleanup to avoid repeating that color resolution when the relevant inputs haven't changed.

If you'd prefer changes like this to be backed by measured slowdown before merging, I'm happy to close this or keep it scoped to Chip rather than updating similar code elsewhere.

@satya164
Copy link
Copy Markdown
Member

@popsiclelmlm let's close it for now as it'll result in conflicts with ongoing work unnecessarily. happy to merge changes later if we can measure it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants