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

combobox: Fix onSelectionChange type declaration #5852

Merged
merged 2 commits into from
Feb 16, 2024

Conversation

acr92
Copy link
Contributor

@acr92 acr92 commented Feb 12, 2024

onSelectionChange can be invoked with a null key if the user clears the input.

// Set selectedKey to null when the user clears the input.
// If controlled, this is the application developer's responsibility.
if (inputValue === '' && (props.inputValue === undefined || props.selectedKey === undefined)) {
setSelectedKey(null);
}

This in turns triggers onSelectionChange with a null value, even though the type contract doesn't allow that.

Here's my stacktrace:
image

SelectUser is my component, which sets defaultItems, selectedKey and onSelectionChange.

✅ Pull Request Checklist:

  • N/A Included link to corresponding React Spectrum GitHub Issue.
  • N/A - Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • N/A - Filled out test instructions.
  • N/A - Updated documentation (if it already exists for this component).
  • N/A - Looked at the Accessibility Practices for this feature - Aria Practices

N/A for all as this is just a correction of the type declaration.

onSelectionChange can be invoked with a null key if the user
clears the input.

packages/@react-stately/combobox/src/useComboBoxState.ts line 225-229
@reidbarber
Copy link
Member

Thanks for the PR! If you sign the CLA, then close and re-open this PR, it should trigger a re-build that passes CI.

@acr92 acr92 closed this Feb 12, 2024
@acr92 acr92 reopened this Feb 12, 2024
Copy link
Member

@snowystinger snowystinger left a comment

Choose a reason for hiding this comment

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

Thanks for the PR
Initially I thought this should be moved up to SingleSelection. However, Picker and StepList both extend it as well, and those have selectionChanges which cannot be null.

This falls into our TS Strict #3183 as well

@reidbarber reidbarber merged commit aa83c12 into adobe:main Feb 16, 2024
26 checks passed
@acr92 acr92 deleted the acr/combobox_fix_typing branch February 16, 2024 22:14
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.

None yet

3 participants