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

Don't return null if the last item is manually removed #4117

Closed
wants to merge 3 commits into from
Closed

Don't return null if the last item is manually removed #4117

wants to merge 3 commits into from

Conversation

Mecit
Copy link

@Mecit Mecit commented Jul 9, 2020

Fixes the Invalid attempt to spread non-iterable instance which occurs when items are removed from the input one by one.

Fixes the `Invalid attempt to spread non-iterable instance` which occurs when items are removed from the input one by one.
@changeset-bot
Copy link

changeset-bot bot commented Jul 9, 2020

⚠️ No Changeset found

Latest commit: 21ede43

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jul 9, 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 21ede43:

Sandbox Source
react-codesandboxer-example Configuration

@bladey
Copy link
Contributor

bladey commented Jul 16, 2020

Thanks @Mecit!

@bladey bladey added pr/bug-fix PRs that are specifically addressing a bug pr/needs-review PRs that need to be reviewed to determine outcome labels Jul 16, 2020
Copy link
Collaborator

@ebonow ebonow left a comment

Choose a reason for hiding this comment

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

Great work. Perhaps you can add one small fix as well while this is open.

Can you add the following after line 34: (or something equivalent)

if (value.find(val => val && val.value === inputValue)) return;

There is another bug present in this example if you try to add a duplicate word.

@ebonow ebonow mentioned this pull request Dec 11, 2020
@Methuselah96
Copy link
Collaborator

Methuselah96 commented Dec 18, 2020

Thanks for the PR! It makes sense to merge this, but hopefully we can also fix the underlying issue in the code soon too (see #3632 and #4339).

@Methuselah96 Methuselah96 added this to the 3.3 milestone Jan 13, 2021
@Methuselah96
Copy link
Collaborator

Methuselah96 commented Jan 20, 2021

@Mecit Seems like ESLint is failing. Perhaps it doesn't like the ??? Maybe an ESLint upgrade would fix it.

@Mecit
Copy link
Author

Mecit commented Jan 20, 2021

@Methuselah96 Yes, it seems ESLint 7.2.0 is needed for the nullish coalescing feature. If upgrading the package would be too bothersome, we can consider using || instead. { value: value || [] } would yield the same result.

@Methuselah96
Copy link
Collaborator

Methuselah96 commented Jan 20, 2021

@Mecit It's probably easier to go with || for now thanks! I'll work on getting ESLint upgraded in a separate PR (it looks like there are a few errors that need to be resolved after upgrading).

@Methuselah96
Copy link
Collaborator

@Mecit This should actually be no longer necessary with the merge and release of #4339. Apologies for the back-and-forth on this and it no longer being necessary in the end. We really appreciate the contribution!

@ebonow ebonow removed this from the 4.1 milestone Jan 22, 2021
@Mecit
Copy link
Author

Mecit commented Jan 22, 2021

@Methuselah96 no problem. I'm aware my commit was a quick fix to the problem and I'm glad the underlying issue was resolved in a more convenient way.

@Mecit Mecit deleted the patch-1 branch January 23, 2021 02:19
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/needs-review PRs that need to be reviewed to determine outcome
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants