Skip to content

Remove memoization of buildMenuOptions #4388

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

Methuselah96
Copy link
Collaborator

@Methuselah96 Methuselah96 commented Jan 20, 2021

Resolves #4259.
Alternative to #4349.

Background

Memoization of buildMenuOptions was added in #3868 as part of the 3.1.0 release. As noted in the PR:

I've made buildMenuOptions memoized, so that it does not run the function again if the props that the function uses didn't change.

and

I can remove the memoizing part to make the build pass, if that would speed up the merging and reviewing of this PR. It would be a bit more inefficient, but it would fix the bug.

Proposal

Reasons to remove the memoization:

  1. The memoization was not essential to the fix in question.
  2. It has since caused other issues including onCreateOption is not called, fails at valueArray[valueArray.length - 1] === newOption #3988 (already resolved) and isOptionDisabled does not update disabled items between renders #4259.
  3. One of the values that would require buildMenuOptions to be recomputed is the inputValue prop. Since the inputValue prop is likely to change quickly, the memoization will not improve performance when the user is typing. Instead performance optimizations should be focused elsewhere.
  4. The alternative is undesirable:
    a. There are a lot of props that are used in buildMenuOptions.
    b. Each of the functions now need to take the props as an argument because buildMenuOptions is used in componentWillReceiveProps.
    c. It would be easy to miss updating the list of props that are used in buildMenuOptions and it is not trivial to find all the props in use.

This would be included as part of the 4.0 release since there is a possible regression in performance. If the change proves to be undesirable, it should be easy to revert.

@changeset-bot
Copy link

changeset-bot bot commented Jan 20, 2021

🦋 Changeset detected

Latest commit: ff74140

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
react-select Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@Methuselah96 Methuselah96 added this to the 4.0 milestone Jan 20, 2021
@codesandbox-ci
Copy link

codesandbox-ci bot commented Jan 20, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit ff74140:

Sandbox Source
react-codesandboxer-example Configuration
little-glade-s2m9u Issue #4259

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.

isOptionDisabled does not update disabled items between renders
2 participants