react-select icon indicating copy to clipboard operation
react-select copied to clipboard

Fix pop-value being sent for undefined values.

Open leonaves opened this issue 1 year ago • 6 comments

Because of this array access:

https://github.com/JedWatson/react-select/blob/884f1c42549faad7cb210169223b427ad6f0c9fd/packages/react-select/src/Select.tsx#L1091

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):

https://github.com/JedWatson/react-select/blob/edf5265ee0158c026c9e8527a6d0490a5ac2ef23/packages/react-select/src/types.ts#L138-L142

leonaves avatar Sep 18 '24 10:09 leonaves

🦋 Changeset detected

Latest commit: 5c9fe3e4b3b2f9bf615deafbaa40cc399009ad7b

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

changeset-bot[bot] avatar Sep 18 '24 10:09 changeset-bot[bot]

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.

codesandbox-ci[bot] avatar Sep 18 '24 10:09 codesandbox-ci[bot]

@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! 🙏

leonaves avatar Sep 18 '24 12:09 leonaves

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.

Methuselah96 avatar Sep 18 '24 12:09 Methuselah96

@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.

leonaves avatar Sep 18 '24 13:09 leonaves

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.

Methuselah96 avatar Sep 18 '24 13:09 Methuselah96

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

👀 😄

leonaves avatar Sep 19 '24 13:09 leonaves

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.

lights-a5 avatar Sep 23 '24 15:09 lights-a5

@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

kibertoad avatar Sep 23 '24 16:09 kibertoad

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!

lights-a5 avatar Sep 23 '24 17:09 lights-a5