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

add stricter Omit helper type #30825

Closed
5 tasks done
seansfkelley opened this issue Apr 9, 2019 · 28 comments
Closed
5 tasks done

add stricter Omit helper type #30825

seansfkelley opened this issue Apr 9, 2019 · 28 comments
Labels
Declined The issue was declined as something which matches the TypeScript vision Suggestion An idea for TypeScript

Comments

@seansfkelley
Copy link

Search Terms

omit strict

Suggestion

The new Omit type does not restrict the omitted keys to be keys actually present on the given type. There should be some avenue to express "omit, but only with keys that are present", likely either a change to Omit or some other Omit-like type.

Use Cases

Copied from pelotom/type-zoo#31.

The benefit that a stricter type has is primarily:

  • Preventing typos.
  • Allowing the compiler to pick up on rename refactors automatically.

Currently, permissive Omit acts as a "barrier" that prevents rename refactors from passing through, which means that any such refactor generates whole bunches of errors that have to be manually fixed. If the field in question is optional, this can actually introduce bugs.

And some further color:

I generally use Omit with string literal unions (as opposed to, say, generic types that extends string), because I often use them for defining higher-order React components that wrap another component except for this one prop. As such, in my use case, I never want a permissive Omit.

Examples

interface ImplementationDetailProps {
  publiclyVisibleFoo: Foo;
  filePrivateBar: Bar;
}

class ImplementationDetail extends React.Component<ImplementationDetailProps> { ... }

export type PublicProps = Omit<ImplementationDetailProps, "filePrivateBar">;

export class Public extends React.Component<PublicProps> { ... }

Checklist

My suggestion meets these guidelines:

  • This wouldn't be a breaking change in existing TypeScript/JavaScript code
  • This wouldn't change the runtime behavior of existing JavaScript code
  • This could be implemented without emitting different JS based on the types of the expressions
  • This isn't a runtime feature (e.g. library functionality, non-ECMAScript syntax with JavaScript output, etc.)
  • This feature would agree with the rest of TypeScript's Design Goals.
@RyanCavanaugh RyanCavanaugh added In Discussion Not yet reached consensus Suggestion An idea for TypeScript labels Apr 10, 2019
@sindresorhus
Copy link

sindresorhus commented Apr 10, 2019

I was disappointed when I saw that the Omit type added in #30552 was not the strict one suggested in #30455, and what most third-party type libraries provide. Like @seansfkelley mentioned, there are many benefits of making it strict. One additional benefit is the ability for everyone using Omit in these type libraries to use the built-in one instead. This will not be possible if the built-in one is not strict.

// @BendingBender @CvX

@nmain
Copy link

nmain commented Apr 10, 2019

Currently, permissive Omit acts as a "barrier" that prevents rename refactors from passing through

Could this be fixed instead?

@seansfkelley
Copy link
Author

seansfkelley commented Apr 10, 2019

Currently, permissive Omit acts as a "barrier" that prevents rename refactors from passing through

Could this be fixed instead?

That would help a lot, I think, but doesn't catch cases where someone does a manual refactor or just outright typos a field name and ends up exposing the wrong type to a consumer (i.e., a type that has fields that should not be visible).

Edit: I will say, I actually don't understand the use-case for permissive Omit at all -- I've never wanted it, but that might be a function of the fact that I'm not generally juggling generic extends string omits (e.g. type Foo<T extends string> = Omit<Bar, T>) but rather literal omit (e.g. type Foo = Omit<Bar, "field">).

@DanielRosenwasser DanielRosenwasser added the Declined The issue was declined as something which matches the TypeScript vision label Apr 16, 2019
@DanielRosenwasser
Copy link
Member

It seems like the constrained Omit type is going to make at least half of users unhappy based on declarations within DefinitelyTyped. We've decided to go with the more permissive built-in which your own constraints can build on.

@sindresorhus
Copy link

I wish I hadn't encouraged opening this issue in the first place. It just made the situation worse than before as the Omit name is now taken by TS, so we have to rename our strict versions to prevent confusion...

@SamVerschueren
Copy link

SamVerschueren commented Apr 17, 2019

It seems like the constrained Omit type is going to make at least half of users unhappy based on declarations within DefinitelyTyped.

And now half of the users are unhappy because it's loose. I totally understand that decisions like this are tough, but you can't make everyone happy. Sometimes you have to make people unhappy for the greater good. And in my opinion, strict typing is the greater good.

The current situation is that everyone is still using a custom Omit, e.g. StrictOmit which again should be defined everywhere. Hence, we're back to start.

@andyfleming
Copy link

I like having a loose Omit option available, but I agree it would be nice to see Omit stay consistent with how the community uses and understands it today. Providing both seems like a win-win.

Maybe we could find an alternate name for it?

  • OmitIf
  • Exclude
  • Remove
  • Without

@RyanCavanaugh
Copy link
Member

it would be nice to see Omit stay consistent with how the community uses and understands it today.

I have to clear up this misconception. There were 12 different definitions of Omit on DT and the two most popular definitions differed on whether or not to constraint the key:

(hit count, definition)
15
type Omit<T, K extends keyof T> = Pick<T, Exclude<keyof T, K>>
13
type Omit<T, K> = Pick<T, Exclude<keyof T, K>>;
11
type Omit<T, K extends keyof T> = Pick<T, ({ [P in keyof T]: P } & { [P in K]: never } & { [x: string]: never, [x: number]: never })[keyof T]>;
3
type Omit<T, K extends keyof any> = Pick<T, Exclude<keyof T, K>>;
2
type Omit<T, K extends keyof T> = Pick<T, Diff<keyof T, K>>;
2
type Omit<T, K> = { [key in Exclude<keyof T, K>]: T[key] };
2
type Omit<T1, T2> = Pick<T1, Exclude<keyof T1, keyof T2>>;
1
type Omit<T, E extends keyof T> = { ...
1
type Omit<T, K extends keyof any> = T extends any ? Pick<T, Exclude<keyof T, K>> : never;
1
type Omit<T, K extends keyof T> = Pick<T, ({ [P in keyof T]: P } & { [P in K]: never } & { [x: string]: never })[keyof T]>;
1
type Omit<T, K extends keyof T> = T extends any ? Pick<T, Exclude<keyof T, K>> : never;
1
type Omit<T, K extends string> = Pick<T, Exclude<keyof T, K>>;

@RyanCavanaugh
Copy link
Member

You can pick at the numbers and try to declare a democratic majority or something, but the reality is that only one definition doesn't break a substantial portion of people.

Moreover there is nothing wrong with passing non-keyof T arguments to K:

type Omit1<T, K> = Pick<T, Exclude<keyof T, K>>;
type Omit2<T, K extends keyof T> = Pick<T, Exclude<keyof T, K>>;

// Can't use Omit2 here
declare function combineSpread<T1, T2>(obj: T1, otherObj: T2, rest: Omit1<T1, keyof T2>): void;

type Point3d = { x: number, y: number, z: number };
declare const p1: Point3d;
// OK
combineSpread(p1, { x: 10 }, { y: 5, z: 2 });
combineSpread(p1, { x: 1, y: 3 }, { z: 2 });
// Error
combineSpread(p1, { x: 10 }, { z: 2 });

@seansfkelley
Copy link
Author

Moreover there is nothing wrong with passing non-keyof T arguments to K

This is true, and why the original suggestion was worded to allow for "some other Omit-like type". That said, as I noted in an earlier comment:

I actually don't understand the use-case for permissive Omit at all

Which is still mostly true, though your provided example seems like a thing I would eventually write at some point, I suppose. Currently, I use type-zoo's OmitStrict everywhere and therefore, the name collision doesn't matter to me, practically speaking.

I always default to strict rather than lenient, and if the standard library doesn't want to, that's okay. I simply figured that if the standard library was going to try to be helpful by providing what until recently was a de facto community-standard type, it would want to address all the related use-cases, and I saw an opportunity to roll more of type-zoo into the standard library where I think it would be very helpful.

@andyfleming
Copy link

andyfleming commented May 17, 2019

@RyanCavanaugh I appreciate that you want to look at the facts as opposed to passing opinion. Totally fair. That said, I want to make sure I'm following which packages you're talking about.

The two top packages I see, type-fest and typical, both look like they require the key to be present (if I'm reading them correctly).

Package Usage

Screen Shot 2019-05-17 at 2 33 10 PM

Screen Shot 2019-05-17 at 2 33 19 PM

See stats from npmtrends.com

Definitions

type-fest definition:

export type Omit<ObjectType, KeysType extends keyof ObjectType> = Pick<ObjectType, Exclude<keyof ObjectType, KeysType>>;

typical definition:

export type Omit<T, K extends keyof T> = T extends any ? Pick<T, Exclude<keyof T, K>> : never;

@andyfleming
Copy link

andyfleming commented May 17, 2019

You can pick at the numbers and try to declare a democratic majority or something, but the reality is that only one definition doesn't break a substantial portion of people.

@RyanCavanaugh I think this is a reasonable point.

@andyfleming
Copy link

Also worth noting that, historically, the TypeScript docs have always shown examples as the strict version.

We did not include the Omit<T, K> type because it is trivially written as Pick<T, Exclude<keyof T, K>>

(from https://www.typescriptlang.org/docs/handbook/advanced-types.html)

@andyfleming
Copy link

All that said, I agree that Omit should stay loose and OmitStrict (or an alternatively named type helper) should be added in addition.

@leoyli
Copy link

leoyli commented May 19, 2019

This is really misfortune to be declined. Can someone tell me what is the motivation why we add Omit when the decision was made at 2.8 saying it's trivial??? It's definitely NOT as trivial. Also, should the doc also get updated to avoid confusion for the future developers?

In the release note, it's been said that TS core team finally realize it's so common so hoping to standardize its implementation for the community. I see it's a great move to make our life easier, but then I see it just get shipped sloppily. 🙁

@RyanCavanaugh, I did surprise you dig into the DT and count these implementations and use it justify something that is shaky. I do appreciate this empathy to ensure not making too much noisy while ensure everyone can be happy whenever it is possible. But I do hope you and other members in the core team to rethink about the Omit implementation. If you guys really want to make it useful while ensure the backward compatibility then perhaps we should get a much different and less frequent name like it was did on choosing naming for Exclude over Diff.

Also, we can consider Pick, the closer friends of Omit, you can see how similarity they are and how they can be complimentary to each other.

Pick: pick members from a group to get a sub-group;
Omit: omit members in a group to get a sub-group;

It looks pretty nature and intuitive when we see them together, isn't it? But how come Omit is a permissive type while Pick isn't??? Reminder: Pick do constrains the second typed variable.

When we make comparison, compare Omit with Exclude is like compare egg with chicken. Exclude is comparable with Extract not Omit, which should be Pick, IMHO.

@leoyli
Copy link

leoyli commented May 19, 2019

Uh... I see where this weird (Omit v.s. Exclude/Extract) comparison came from (#30738). I think it is not good to post my comment there so I did it here. Here is how I justify the API surface for how the built-in helper types could be:

              non-permissive        permissive

anti-selective   Omit* <-----------> Exclude
                  ^                     ^
                  |                     |
                  v                     v
selective        Pick <------------> Extract

For the above relationship and reason, I really believe Omit shouldn't be permissive. If it has to be, please, please, please, find another less noisy naming for it... and so the 2.8 doc don't need to get updated too, win-win?! 🤔.


UPDATE: why this is important:

In react, we sometimes want to write glue component to bridge two libraries, then we will have:

type ConstrainedComponentProps = BasicProps &
  Omit<LibAProps, 'A' | 'B'> &
  Pick<LibBProps, 'C' | 'D'>

That is to say, Pick give some good IDE intellisense to catch typo, but the current Omit didn't so it can have leaky abstraction that confused the consumed developers. Such real world use-case is really not in-common. In practice, anyone use Pick together with Omit would agree that making Omit permissive is a smell decision.

@G-Rath
Copy link

G-Rath commented May 30, 2019

Personally I agree that Omit should be strict, given it's the opposite of Pick (@leoyli pretty much nailed it).

You can pick at the numbers and try to declare a democratic majority or something, but the reality is that only one definition doesn't break a substantial portion of people

which only becomes a reality when those people both update to 3.5, and remove their definition of Omit.

Case and point, I can do this no problem:

type Pick = string;

const picked: Pick = 'hello';

I mean given that you have to update eitherway, you could have easily made both parties happy by implementing LooseOmit & StrictOmit, and then decided if you wanted to make a call to which one gets to be "Omit", if any.

only one definition doesn't break a substantial portion of people

That's assuming that everyone not using StrictOmit is doing so loosely. You might have looked into this more than just the numbers, but if you've not then I don't think they're that valuable, as it depends on how they're actually being used.

For example, it's quite reasonable the people using LooseOmit have been doing so in a "strict" sense i.e their typings don't require the looseness, it just happens to be how they wrote the typing at the time.


However, in saying all this, for me my whole actual problem with having LooseOmit, is the lack of intellisense. It's just plain annoying, especially given Pick, and that the whole reason you'd use a string literal with Omit would be for a defined property (i.e LooseOmit doesn't make any sense when you're omitting string literals)

I've found that in WebStorm intellisense if you have type Omit<T, K extends keyof T | any> = Pick<T, Exclude<keyof T, K>>; but sadly that's not the case in playground or VSCode.

Off the top of my head (which is a head that has little idea how the internals of TS work for generating intellisense), could the intellisense somehow ignore any in unions? i.e so that keyof T | any would become keyof T for the intellisense only.

That wouldn't be a breaking change, since (at least imo) what is suggested for any is unpredictable anyway b/c of the nature of any: you can suggest anything.


Finally, a crazy idea: what about omit.strict and omit.loose libs? I don't know enough about how libs work internally & in dependencies to know if that'll just straight up break, say if you uses lib: [ 'omit.strict' ] and installed a DT package that uses loose, but that would let us pick between the two.

@RyanCavanaugh RyanCavanaugh removed the Declined The issue was declined as something which matches the TypeScript vision label May 31, 2019
@zackschuster
Copy link

is it possible that a future release (3.6, 4.0, etc.) could tighten the strictness? i can understand the "open arms" approach here, but i really would prefer a stricter version of the type at some point for basically all the reasons mentioned so far.

@OliverJAsh
Copy link
Contributor

If you want to enforce that no-one on your project accidentally uses the built-in Omit, here's an ESLint rule for that:

{
  rules: {
    '@typescript-eslint/ban-types': [
      'error',
      {
        types: {
          Omit: "Prefer `OmitStrict`.",
        },
      },
    ],
  }
}

@whatisaphone
Copy link

It seems like the constrained Omit type is going to make at least half of users unhappy based on declarations within DefinitelyTyped.

It looks like the typescript team might have accidentally found a technical solution to what is actually a people problem: what do users want this type to be? What are programmers trying to express when they write Omit<Props, 'className'>?

There are plenty of conceivable reasons an Omit in DT might not constrain the keys. One is low-quality or under-maintained packages. Another is that the definition was written for TypeScript 2.0 which didn't have keyof yet. I think the latest version of the stdlib should be held to a higher standard than miscellaneous DT packages. I hope we can find a path forward towards improving this situation.

@G-Rath
Copy link

G-Rath commented Aug 21, 2019

Just an FYI: WebStorm 2019.2.2 will ship WEB-40482 that'll mean it will provide intellisense for the second argument of Omit based off the first argument:

Artificially supplied Omit's second argument with the expected type containing all the keys of the first argument.

Now completion, go to declaration, find usages and rename will work for properties referenced in Omit's second argument.

Also, massive shoutout to Anton of the WebStorm team, for his amazing work improving the TypeScript side of things, and for having to put up w/ me throwing tons of TS edge-cases at him 😂

@RyanCavanaugh RyanCavanaugh added Declined The issue was declined as something which matches the TypeScript vision and removed In Discussion Not yet reached consensus labels Aug 21, 2019
@RyanCavanaugh
Copy link
Member

After much discussion, we think libraries providing their own "stricter" versions of Omit (as well as Exclude and other friends) is preferable. The Strict versions of these types are very infectious in terms of forcing upstream constraints and it's not clear that people will wisely choose between them. This can induce some real friction if not carefully managed.

This is actually more true now that Omit is in the lib - if we pick OmitStrict (for which I'm sure there are in turn multiple definitions to pick from) then there's not much in terms of good names left in userspace. There are a few ways to write all of these helper types and letting developers choose the one that matches their own definition of "strict" is the option that's going to maximize individual freedom.

The reaction here and on Twitter to our (entirely defensible IMO) choice of the definition of Omit shows why we're not really excited about picking an OmitStrict that is going to anger some 30-70% of developers because we don't pick their preferred definition. It appears it's only really a good idea to add things to the lib if its definition is entirely unambiguous, and OmitStrict doesn't fit that criteria. In retrospect, neither did Omit, and perhaps we should have just left it out.

Go forth, developers, with your chosen definitions of OmitStrict happy and secure in the knowledge that TS isn't going to stomp on that name 😬

@seansfkelley
Copy link
Author

Apologies for resurrecting this thread, but I did stumble upon a behavioral difference that made me very happy for my usage of OmitStrict. I respect the team's decision to not include it in the standard library, but I thought this example was worth having recorded somewhere, and I don't believe it's been mentioned in previous comments.

I'll let the example speak for itself:

type OmitStrict<T, K extends keyof T> = T extends any ? Pick<T, Exclude<keyof T, K>> : never;

interface Foo {
  foo: string;
}

// Silently produces dangerous {} type.
type T1 = Omit<Foo | undefined, "bar">;

// Complains, regardless of what you provide to the second type parameter.
type T2 = OmitStrict<Foo | undefined, "bar">;

// Complains like you would expect.
type T3 = OmitStrict<NonNullable<Foo | undefined>, "bar">;

// Ah, type safety.
type T4 = OmitStrict<NonNullable<Foo | undefined>, "foo">;

Playground Link

@nelson6e65
Copy link

Thanks for the OmitStrict helper, @seansfkelley.

@devuxer
Copy link

devuxer commented May 20, 2021

@RyanCavanaugh,

I take your point that no matter what you do as far as Omit vs. OmitStrict, people are going to complain, but here's one more suggestion to mull over:

Could there not be a TSConfig option strictOmit? I know there are already a million switches in TSConfig, but this would allow people to opt into a type-checked Omit if that's already how they're using it in their project.

Happy to raise this as a separate issue if you think it's worth considering and doesn't already exist.

@ljharb
Copy link
Contributor

ljharb commented May 20, 2021

@devuxer a flag would mean that you could change the behavior of types on a third-party project, which doesn't seem like it'd work out too well.

@RyanCavanaugh
Copy link
Member

What @ljharb said. Flags like that are just asking for errors to appear ex nihilo in your imported .d.ts files, which no one wants.

codehub0x added a commit to codehub0x/eslint-typescript that referenced this issue Jul 18, 2021
@ejose19
Copy link

ejose19 commented Nov 28, 2021

Couldn't Omit just accept another optional argument strict = false? That way it remains backward compatible and users wouldn't need to create a wrapper (or worse, import a 3rd party library for just StrictOmit)

PoC:

type Omit<
  T,
  K extends S extends true ? keyof T : keyof any,
  S extends boolean = false
> = Pick<T, Exclude<keyof T, K>>;

interface X {
  foo: string;
  bar: string;
}

type A = Omit<X, "baz">;        // Backward compatible, no errors
type B = Omit<X, "baz", true>;  // Error: Type '"baz"' does not satisfy the constraint 'keyof X'

@microsoft microsoft locked as resolved and limited conversation to collaborators Nov 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Declined The issue was declined as something which matches the TypeScript vision Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests