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

SetOptional/SetRequired/SetReadonly: Fix instantiations with index signatures #1014

Conversation

som-sm
Copy link
Collaborator

@som-sm som-sm commented Dec 19, 2024

Fixes #1013

  • Tweaked the implementation of DistributedPick so that it handles any properly
  • Set-* types now use DistributedPick instead of Except/Exclude combination

@som-sm som-sm force-pushed the fix/set-optional-readonly-required-types-with-index-signatures branch from 6b2ba55 to ab827f9 Compare December 19, 2024 08:40
source/distributed-pick.d.ts Outdated Show resolved Hide resolved
source/set-optional.d.ts Outdated Show resolved Hide resolved
@som-sm som-sm force-pushed the fix/set-optional-readonly-required-types-with-index-signatures branch from ab827f9 to 9d7471f Compare December 19, 2024 08:56
@som-sm
Copy link
Collaborator Author

som-sm commented Dec 19, 2024

Note: The updated implementation of DistributedPick introduces subtle changes in it's behaviour, which might be problematic if the goal is to keep DistributedPick fully aligned with the built-in Pick for non-union types.

1. Behaviour with primitives

// Previous behaviour
type Test = Pick<string, 'toUpperCase'>;
//   ^? type Test = { toUpperCase: () => string }
// Updated behaviour
type Test = Pick<string, 'toUpperCase'>;
//   ^? type Test = string

And this happens because homomorphic mapped types pass through primitives unchanged.

2. Picking number from string indexor

// Previous behaviour
type Test = Pick<{ [k: string]: unknown }, number>;
//   ^? type Test = { [x: number]: unknown; }
// Updated behaviour
type Test = Pick<{ [k: string]: number }, number>;
//   ^? type Test = {}

@sindresorhus If you think this might create issues, then I'd create a separate internal Pick type instead of modifying DistributedPick.

@sindresorhus
Copy link
Owner

I think it would be better to create an internal pick type for now.

Do you think the new behavior is the correct one / better? If so, we could consider changing it in v5: #450

Alternatively, if both the old and new behavior makes sense in different situations, we could consider adding an option to the type.

@som-sm
Copy link
Collaborator Author

som-sm commented Dec 19, 2024

It’s hard to determine which behaviour is better, as these are very rare scenarios.

While we could add an option for this, I doubt it'd be very useful—it might just create confusion. Currently, DistributedPick does what it says, it distributes Pick, but introducing additional changes might not be that obvious.

And IMO, the updated behaviour is better in the 1st case, while the previous behaviour is better in the 2nd, so this definitely needs more discussion.

For now, I'll create an internal Pick.

@som-sm som-sm force-pushed the fix/set-optional-readonly-required-types-with-index-signatures branch from 8fe5f99 to 19d58e3 Compare December 19, 2024 17:08
@sindresorhus sindresorhus merged commit cb269ff into sindresorhus:main Dec 25, 2024
12 checks passed
@sindresorhus
Copy link
Owner

@som-sm Nice work! You have done a lot of great work here lately. Would you be interested in being added as a maintainer? No worries if not :)

@som-sm
Copy link
Collaborator Author

som-sm commented Dec 25, 2024

@sindresorhus Yeah sure, I’d be happy to help as a maintainer and contribute further.

@sindresorhus
Copy link
Owner

Awesome 🎉

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.

SetOptional remove properties of types with index signatures
2 participants