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

fix: Pick and Omit over intersection types #31

Merged
merged 2 commits into from
Jun 14, 2019
Merged

Conversation

unional
Copy link
Owner

@unional unional commented Jun 14, 2019

@dragomirtitian

I get the initial implementation of these Pick<> and Omit<> from your comment microsoft/TypeScript#28339 (comment)

I tried it out and found that the UnionKeys<> does not work on intersection types.

Do you see any concern of using keyof T over UnionKeys<>?

@codecov-io
Copy link

codecov-io commented Jun 14, 2019

Codecov Report

Merging #31 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #31   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          15     15           
  Lines          70     70           
  Branches        3      3           
=====================================
  Hits           70     70
Impacted Files Coverage Δ
src/pick.ts 100% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c2af11a...bb011fe. Read the comment docs.

@unional unional merged commit e616ab9 into master Jun 14, 2019
@unional
Copy link
Owner Author

unional commented Jun 14, 2019

🎉 This PR is included in version 1.21.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@dragomirtitian
Copy link

@unional I am concerned about compiler performance, distributive conditional types tend to cause performance issues with large unions. But other than that it should be fine if T is not a union UnionKeys<T> will just be keyof T.

What do you mean UnionKeys does not work with intersections ? Seems to me it works

Thanks for the attribution :)

@unional
Copy link
Owner Author

unional commented Jun 14, 2019

Thanks. I'm debating whether should the user concern with compiler performance when they choose which type to use. IMO it should be about behavior. Should the user need to know about the gritty detail?
That's why for now I keep the name as Pick<> and Omit<>, replacing the functionality of the build-in one if user choose to.

It doesn't work on intersection with generics. You can check it out here

@dragomirtitian
Copy link

dragomirtitian commented Jun 14, 2019

Performance isn't an issue until it is. But I agree functionality is more important.

The reason it does not work on intersection with generics is that conditional types with unresolved type parameters are not expanded, so `UnionKeys<T & Foo> is opaque to the compiler inside the generic function.

One change that can improve this situation is to add keyof T to UnionKeys as a fallback so at least for generics you are not worse of than before.

type UnionKeys<T> = keyof T | (T extends any ? keyof T : never)

sample

@unional
Copy link
Owner Author

unional commented Jun 14, 2019

Thanks, that works! 🌷

Agree with what you said. If we have a way to distinguish between normal unions and discriminative unions, we can fall back to the more performant way, but forcing user to use a different Pick<> and Omit<> because the subject is a discriminative union is a hard call. I can see a lot of frustration and scratching heads because of that.

On a side note, what do you think about:

type UnionKeys<T> = keyof T | (T extends any ? keyof T : never)
// vs
type UnionKeys<T> = keyof T | (T extends T ? keyof T : never)

@dragomirtitian
Copy link

I used to use T extends any (still do out of habit sometimes), but it was pointed out to me in an issue i filed (when extends any was not working as expected in StrictUnion microsoft/TypeScript#30569) that any is 'weird' (I think that should be in the doc "Any is weird" 😂) and is best avoided even in conditional types. So I would go with T extends T or T extends unknown.

@unional
Copy link
Owner Author

unional commented Jun 15, 2019

Got it.
I still wonder if Pick<> and Omit<> should use UnionKeys<>.
I understand that Pick<> and Omit<> should use conditional type so that it can be distributed to the unions.
But I'm not sure about "distributing" K over the keyof T, can't wrap my mind around it.

Based on this example,

it seems like keyof T gives more accurate result. (check x and y at the end)

@dragomirtitian
Copy link

Yeah, if it's not keyof T as a constraint to K the type is not homomorphic anymore, we can get back to it being homomorphic if we use Pick again.

type UnionKeys<T> = keyof T | (T extends T ? keyof T : never)

type Pick2<T, K extends UnionKeys<T>> = T extends T ? Pick<T, K> : never
type Pick3<T, K extends keyof T> = T extends T ? { [P in K]: T[P] } : never

type Foo = { a: string, b: string }
type Boo = { a?: string, b: string }

function foo<T>(i2: Pick2<Foo & T, 'a'>, i3: Pick3<Foo & T, 'a'>): void {
    
}

let x: Pick2<Foo | Boo, 'a'> // Pick<Foo, "a"> | Pick<Boo, "a">
let y: Pick3<Foo | Boo, 'a'> // { a: string } | { a?: string | undefined }

Playground

@unional
Copy link
Owner Author

unional commented Jun 15, 2019

Yes, but that is just trying to push it down to get the type works again with UnionKeys<>.

For this usage, what is the benefit of using UnionKeys<T> vs keyof T?

I can't find an example that using UnionKeys<T> would be better than keyof T in this situation,
and since we are distributing the type anyway, it does not seem to be any performance benefits (in contrast to having a merged type vs union type, which the T extends T ? { [P in K]: T[P] } : never offers).

@dragomirtitian
Copy link

@unional (Sorry saw the comment forgot to respond).

I can't think of any concrete use cases where I would want to Pick from the un-common fields of a union. It is kind of neat you can write:

type UnionKeys<T> = keyof T | (T extends T ? keyof T : never)

type Id<T> = {} & { [ P in keyof T]: T[P]} // just for esthetics
type Pick2<T, K extends UnionKeys<T>> = T extends T ?  Pick<T, Extract<K, keyof T>> : never

type Union = {
    type: "A",
    foo: string
} | {
    type: "B",
    foo: string
    bar: string
}

type PickedUnion = Id<Pick2<Union, "type" | "bar">>
//same as
type PickedUnionResult = {
    type: "A";
} | {
    type: "B";
    bar: string;
}

Link

I think for Omit the case for omitting from just some members might be stronger but that is not based on anything just a gut feeling.

@unional
Copy link
Owner Author

unional commented Jun 19, 2019

Thanks! That's a great example.
The UnionKeys make this usage possible. If using just keyof T, the Pick2<> will complain the bar is not a valid key.

Thanks for all your input and your continuous contribution to SO and to the community. 🌷

@unional unional deleted the loosen-pick-and-omit branch June 29, 2019 07:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants