Skip to content
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

Fix onCreateOption is not always called for Creatable #3990

Merged
merged 7 commits into from
Jan 13, 2021

Conversation

nikitaindik
Copy link
Contributor

Issue: #3988

The issue is caused by "newOption" being recreated as a new object on every render.

@changeset-bot
Copy link

changeset-bot bot commented Mar 28, 2020

🦋 Changeset detected

Latest commit: 64ad645

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

@codesandbox-ci
Copy link

codesandbox-ci bot commented Mar 28, 2020

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 64ad645:

Sandbox Source
react-codesandboxer-example Configuration

@johannesleander
Copy link

Any update on plans to release this fix?

@bcalvignac
Copy link

bcalvignac commented Apr 24, 2020

Yes, I could use this fix, This is causing a new label XXX to appear as "Create XXX"

@bladey bladey added pr/needs-review PRs that need to be reviewed to determine outcome pr/priority PRs that should be addressed sooner rather than later labels May 26, 2020
@sirjuan
Copy link

sirjuan commented Jun 2, 2020

This issue (comparing with reference) sometimes causes onCreateOption to not be called. Instead onChange is called with select-option action and new option object as payload.

Merging and releasing this PR would fix it.

Edit: I tested this solution with our misbehaving component, and it indeed fixed it.

@laznic
Copy link

laznic commented Jun 2, 2020

Could this fix get merged and published please? Thanks in advance!

@makker
Copy link

makker commented Jun 2, 2020

Would be greatly appreciated if this fix could be released! 🙏

@mika-kaki
Copy link

Looking forward for this fix. Thanks for your great work!

@bladey bladey requested review from JedWatson and bladey June 3, 2020 05:19
@bladey bladey added pr/needs-review PRs that need to be reviewed to determine outcome pr/in-review PRs currently in review by maintainers for the next release and removed pr/needs-review PRs that need to be reviewed to determine outcome labels Jun 3, 2020
@bladey
Copy link
Contributor

bladey commented Jun 3, 2020

@JedWatson this looks good to me - pending your review.

@bladey bladey added pr/bug-fix PRs that are specifically addressing a bug pr/priority PRs that should be addressed sooner rather than later and removed pr/priority PRs that should be addressed sooner rather than later labels Jun 3, 2020
@bladey bladey added pr/priority PRs that should be addressed sooner rather than later and removed pr/priority PRs that should be addressed sooner rather than later labels Jun 4, 2020
@nikitaindik
Copy link
Contributor Author

@bladey @JedWatson Hi guys! Do you think we can merge this? :)

@Methuselah96
Copy link
Collaborator

Methuselah96 commented Nov 14, 2020

I've started a fork of react-select. Feel free to resubmit this PR on the fork and we can get it merged and released.

EDIT: 🎉 I've archived the fork now that we've got some momentum in this repo and Jed is involved again. Sorry for the disturbance!

Copy link
Collaborator

@Methuselah96 Methuselah96 left a comment

Choose a reason for hiding this comment

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

@nikitaindik Thanks for the PR!

  1. Can you add a test?
  2. My preference would actually be to change the behavior in Select.js to only memoize the options when they are strictly equal by changing isEqual(newProps.options, lastProps.options) to newProps.options === lastProps.options here (might as well change the other ones from isEqual to === as well). This allows people to use strict equality in their own onChange methods and not have to worry about the implementation detail that the memoization depends on an internal isEqual method. This memoization was introduced in Fix not focusing the selected value on menu open #3868 and I don't believe the memoization was essential to the fix in that PR.

@Methuselah96 Methuselah96 added this to the 3.2 milestone Dec 12, 2020
@nikitaindik
Copy link
Contributor Author

Hi @Methuselah96! I'll have some spare time during the weekend. I'll try to add a test see if I can resolve this isEqual thing.

@Methuselah96
Copy link
Collaborator

@nikitaindik Let me know if you don't have time to make the changes. I can do it myself if you don't have the time.

@nikitaindik
Copy link
Contributor Author

nikitaindik commented Dec 22, 2020 via email

@Methuselah96 Methuselah96 changed the title Fix proposal for #3988: Change newOption check to isEqual Fix onCreateOption is not called for Creatable Jan 13, 2021
@Methuselah96 Methuselah96 changed the title Fix onCreateOption is not called for Creatable Fix onCreateOption is not always called for Creatable Jan 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/bug-fix PRs that are specifically addressing a bug pr/in-review PRs currently in review by maintainers for the next release pr/priority PRs that should be addressed sooner rather than later
Projects
None yet
Development

Successfully merging this pull request may close these issues.

onCreateOption is not called, fails at valueArray[valueArray.length - 1] === newOption
10 participants