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 pop-value being sent for undefined values. #5960

Merged

Conversation

leonaves
Copy link
Contributor

@leonaves leonaves commented Sep 18, 2024

Because of this array access:

const lastSelectedValue = selectValue[selectValue.length - 1];

removedValue could technically be undefined. This happens when the select box is empty and a user hits backspace. It can cause runtime errors in the consuming libraries if the options are objects rather than primitives. This stops those actions being sent.

(The other option would be to update the typing in the below part of the code to mark removedValue as optional, forcing consumers to protect against potential undefined values, but I believe this makes more sense, as we are not removing anything if the set of values is already empty, so sending a pop-value action doesn't really make sense):

export interface PopValueActionMeta<Option> extends ActionMetaBase<Option> {
action: 'pop-value';
removedValue: Option;
name?: string;
}

Copy link

changeset-bot bot commented Sep 18, 2024

🦋 Changeset detected

Latest commit: 5c9fe3e

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

Copy link

codesandbox-ci bot commented Sep 18, 2024

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.

@leonaves
Copy link
Contributor Author

@lukebennett88 @emmatown @Methuselah96 I know this library isn't super actively maintained at the moment, but this is a tiny change with test coverage so I'd love a real quick look and a patch release! Would be really appreciated! 🙏

@Methuselah96
Copy link
Collaborator

Methuselah96 commented Sep 18, 2024

Hmm, this could technically be considered a breaking change, but this is probably our best option. I'll approve and merge if no one objects in the next 24 hours.

@leonaves
Copy link
Contributor Author

@Methuselah96 Yeah I considered that, but as the exact shape of these events isn't actually documented anywhere (as far as I can see) other than the exported types, and the exported types actually declare that removedValue will never be undefined, I'd say it's fine to call it a patch.

@Methuselah96
Copy link
Collaborator

Unfortunately react-select is popular enough that Hyrum's Law is in full effect. If it turns out people were depending on this, and there's on other sufficient solution, I'd be quick to revert it.

@leonaves
Copy link
Contributor Author

I'll approve and merge if no one objects in the next 24 hours.

👀 😄

@Methuselah96 Methuselah96 merged commit dd740ce into JedWatson:master Sep 19, 2024
6 checks passed
@github-actions github-actions bot mentioned this pull request Sep 19, 2024
@lights-a5
Copy link

Commenting here that this change broke my tests for some reason this morning. I haven't been able to delve too deeply into it or to find out what I'm doing that is breaking it. I can confirm that it was working on version 5.8.0 and not 5.8.1.

@kibertoad
Copy link

@lights-a5 Can you check if your tests are correct in this case? maybe they were checking that the wrong behaviour is wrong and actually need to be adjusted

@lights-a5
Copy link

The problem is me it turns out. The require for Select had a d3 in its name and my jest config has a moduleNameMapper mapped from d3 to a d3 library. The only change from you was the slug changed in the dist that included d3 in it.

Just bad regex on my part. Sorry for the alarm!

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.

4 participants