Skip to content

feature(u.distributiveomit): Add U.DistributiveOmit type #75

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

Merged
merged 2 commits into from
Dec 11, 2019

Conversation

Andarist
Copy link
Contributor

🎁 Pull Request

  • Used a clear / meaningful title for this pull request
  • Tested the changes in your own code (on your projects)
  • Added / Edited tests to reflect changes (tst folder)
  • Have read the Contributing part of the Readme
  • Passed npm test

Is there any breaking changes?

  • Yes, I changed the public API & documented it
  • Yes, I changed existing tests
  • No, I added to the public API & documented it
  • No, I added to the existing tests
  • I don't know

Anything else worth mentioning?

A lot more of similar types could be added (for example DistributiveOptional) but I'm not yet sure what do you think about this one.

@Andarist Andarist requested a review from millsp as a code owner December 10, 2019 21:55
@Andarist
Copy link
Contributor Author

I guess when its already under U namespace its ok to call it just Omit. Was using this a standalone type previously and had to name it differently - hence weirdo DistributiveOmit 😉

@millsp millsp merged commit 5a46b9a into millsp:master Dec 11, 2019
@millsp
Copy link
Owner

millsp commented Dec 11, 2019

Hey @Andarist, thanks for your contrib'.

I added U.Pick as well. And since this falls into the U category, I made possible to pass tuples (to keep the parity with the rest of the tools), you can check the changes out.

Thanks!

@Andarist Andarist deleted the pr/u-distributiveomit branch December 11, 2019 17:31
@millsp
Copy link
Owner

millsp commented Dec 19, 2019

Hey @Andarist, I got an after thought

I was thinking about your distributive Omit & Pick to be the default:

  • I don't see any case where we would not want this feature
    (on object unions, current O.Pick gives a pointless {})

  • Mapped types distribute unions (always), but not this one
    (I find this behavior inconsistent and confusing)

  • My goal is to smooth the ts experience with this library

  • This might have very few side effets, small breaking change

@millsp
Copy link
Owner

millsp commented Dec 19, 2019

However, the U.Omit & U.Pick are still different. They are able to handle Any kind of input, and because of that, I will move them to Any (but won't introduce breaking changes for this). So Any is becoming the home for these kind of tools, the ones that can handle any input.

Just leaving these notes to you, as we worked together on this

@Andarist
Copy link
Contributor Author

Mapped types distribute unions (always), but not this one

I guess this is because those are not actually implemented using mapped types (?). But I actually don't know that much about how TS works in those more complex scenarios under the hood. You certainly have much broader knowledge about this stuff.

(I find this behavior inconsistent and confusing)

Yeah, I'm wondering why this works in a way it works by default 🤔

Just leaving these notes to you, as we worked together on this

I appreciate it.

@millsp
Copy link
Owner

millsp commented Dec 19, 2019

Yeah, I'm wondering why this works in a way it works by default 🤔

They are mapped types (Pick is one). But what breaks the distribution here is keyof which does not distribute by default, most likely to prevent excessive combined type distribution (aka very large unions).

type test = keyof ({a: 1, b: 2} | {a: 3, c: 4}) // 'a'

Because of that, you'd only be able to pick the a field, the overlapping fields; which is most likely not what we want.

Also, the TypeScript team did not enable this by default because they wanted to leave the type distribution opt-in, it distributes by using extends or a mapped type. Here's a quote from microsoft/TypeScript#28339:

  • We definitely can't change the behavior now

  • Desirability of distributivity/mapping is entirely scenario-dependent; current defaults exist to a) do the right thing "most" of the time and b) allow opting in to the other behavior

    • The defaults here only seem "bad" if you ignore all the cases where they work like you expect them to

Unfortunately, we cannot simply replace keyof with U.Keys everywhere to try to make the distributivity smooth. If we were to do this we would run into problems on mapped types, like losing readonly and ? on objects for instance.

@Andarist
Copy link
Contributor Author

Problem with TS is that this behavior is not described well anywhere and thus it’s not intuitive. Without stumbling upon some github issues or SO answers one cannot simply know what distributes and what doesnt. Also - I completely dont understand why a conditional type makes this work 🤷‍♂

@millsp
Copy link
Owner

millsp commented Dec 21, 2019

Yeah, it is quite complex to type javascript anyways. This is described in the handbook, but probably not emphasized enough (I agree). The type system is just an API to produce types, with its own syntax, which is very basic & rough. That is why my goal is to provide a smoother user experience.

@millsp
Copy link
Owner

millsp commented Dec 21, 2019

I hope that one day type providers will make their way through. This way we will be able to program the types (like in ts-toolbelt) but in ts/js directly (with far better performance). But this is still in discussion/needing proposal, and is not a milestone yet (and could also never happen).

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.

2 participants