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

[select] fix: mark items arrays as readonly where appropriate #5171

Merged

Conversation

maclockard
Copy link
Contributor

Fixes #4976

Checklist

  • Includes tests
  • Update documentation

Changes proposed in this pull request:

Added readonly annotations to select array props

Reviewers should focus on:

The types

Screenshot

packages/select/src/common/predicate.ts Outdated Show resolved Hide resolved
packages/docs-theme/src/components/navigator.tsx Outdated Show resolved Hide resolved
packages/select/src/common/listItemsProps.ts Outdated Show resolved Hide resolved
@maclockard maclockard mentioned this pull request Mar 16, 2022
2 tasks
@maclockard maclockard force-pushed the maclockard/readonly-select-items branch from 5ccb273 to 8fcd017 Compare March 17, 2022 17:20
@adidahiya adidahiya changed the title Readonly select items prop [select] fix: mark items arrays as readonly where appropriate Mar 17, 2022
@adidahiya adidahiya merged commit dc11929 into palantir:develop Mar 17, 2022
adidahiya added a commit that referenced this pull request Mar 18, 2022
@adidahiya
Copy link
Contributor

I have to revert this, it's causing downstream compilation errors:

[error] TypeScript error packages/[redacted]/filterable-select.tsx:25:9 - error TS2345: Argument of type 'readonly T[]' is not assignable to parameter of type 'T[]'.
  The type 'readonly T[]' is 'readonly' and cannot be assigned to the mutable type 'T[]'.

25         items,
           ~~~~~

@maclockard
Copy link
Contributor Author

@adidahiya curious if that's actually coming from using a Select/QueryList/etc component or if its coming from downstream consumer reusing IListItemsProps/ISelectProps/etc for their own components?

I suspect it's probably the second looking at the file name. If that's the case, seems easy enough to only apply the change for types blueprint components use, and leave the exported types alone?

@adidahiya
Copy link
Contributor

It is the latter, "downstream consumer reusing ISelectProps for their own components". I'm open to an alternative fix if you want to send a PR. I'll test it downstream before merging.

@maclockard
Copy link
Contributor Author

hmmm, now that I think about it, my potential solution to that problem kinda falls under 'no one wins' territory since folks trying to get the actual prop types will have trouble.

I think it makes sense to do this as a breaking change as a part of v5. That way we could also fix the callback types as well. What does queuing a change for v5 look like?

@adidahiya
Copy link
Contributor

What does queuing a change for v5 look like?

I'm not ready for this yet, but i'll have a process for this ready in the next few weeks. Stay tuned

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.

Select's types don't accept readonly item arrays
2 participants