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

Omit seems broken on type extending Record #50638

Closed
Alfred-Mountfield opened this issue Sep 5, 2022 · 9 comments
Closed

Omit seems broken on type extending Record #50638

Alfred-Mountfield opened this issue Sep 5, 2022 · 9 comments

Comments

@Alfred-Mountfield
Copy link

Bug Report

πŸ”Ž Search Terms

omit, extend, Record, &

πŸ•— Version & Regression Information

  • This is the behavior in every version I tried, and I reviewed the FAQ for entries about Omit and Record types

⏯ Playground Link

Playground link on nightly with relevant code

πŸ’» Code

type Foo = {
  bar: string;
  baz: number;
} & Record<string, any>

interface Foo2 extends Record<string, any> {
  bar: string;
  baz: number;
} 

type FooTest = Omit<Foo, "bar">
type Foo2Test = Omit<Foo2, "bar">

πŸ™ Actual behavior

Both FooTest and Foo2Test resolve as being:

type X = {
    [x: string]: any;
}

πŸ™‚ Expected behavior

I would expect the resolved type to be equivalent to:

type X = {
    baz: number;
    [x: string]: any;
}
@whzx5byb
Copy link

whzx5byb commented Sep 5, 2022

Duplicate of #31153

@Alfred-Mountfield
Copy link
Author

Thanks for finding that! My search terms didn't surface it. The issue seems to be closed, even though I don't see a clean working solution under the current version of TypeScript.

It still seems to me that this is a bug with Omit, at the very least it's not working in the way I think is desirable, or how the docs suggest it should work.

Happy to close this if it is viewed as a duplicate and resolved, but #31153 didn't have a reason for being marked as completed, from what I can see, just a reason for why the behaviour is the way it is.

@MartinJohns
Copy link
Contributor

MartinJohns commented Sep 5, 2022

It still seems to me that this is a bug with Omit, at the very least it's not working in the way I think is desirable, or how the docs suggest it should work.

It's working as intended, not a bug. The linked issue provides a working workaround. The TypeScript team won't introduce new implementations for Omit<> because it would break existing usages.

Happy to close this if it is viewed as a duplicate and resolved, but #31153 didn't have a reason for being marked as completed, from what I can see, just a reason for why the behaviour is the way it is.

Because Omit<> is using keyof, and that's the intended way keyof behaves with index signatures. See #42436.

@Alfred-Mountfield
Copy link
Author

Thanks for the response, I'm closing this issue then :)

Understood about it working as intended, and the reasoning about not giving a breaking change. As a user, and a reader of the docs, I have to say this doesn't seem desirable, even if it's not a bug. This seems to non-insignificantly decrease the usability of the Omit type when Record types are quite common.

Because Omit<> is using keyof, and that's the intended way keyof behaves with index signatures.

I appreciate that, that's what I meant by "a reason for why the behaviour is the way it is". I understand how it behaves the way it does, but that seems to be a description of an implementation detail, not a description of why "Omit" should do that. It seems desirable for Omit to work as it intuitively is described to, by allowing the same type except for certain properties.

"because it would break existing usages" somewhat makes sense to me if that is the decision the TypeScript team has made.

@whzx5byb
Copy link

whzx5byb commented Sep 5, 2022

@Alfred-Mountfield

I'm not sure what do you mean by "a clean working solution" but if you are talking about the issue mentioned in #31153 (comment), it could be improved by using a util type. See my latest comment in that post.

@MartinJohns
Copy link
Contributor

I have to say this doesn't seem desirable, even if it's not a bug.

To be fair, I think the entire data structure you propose is undesirable. It's unsound and can lead to hard-to-find bugs.

For example:

const data: Foo = { bar: '', baz: 0 };
const key = "bar" as string;
data[key] = true;
data.bar; // Type is string, actually a boolean.

@Alfred-Mountfield
Copy link
Author

@whzx5byb

I'm not sure what do you mean by "a clean working solution" but if you are talking about the issue mentioned in #31153 (comment), it could be improved by using a util type. See my latest comment in that post.

That is an awful lot of code to be able to remove a single key from a type. To be clear I'm not ungrateful, I appreciate there being a solution. It just seems like that shouldn't be necessary when it seems like that's what Omit says it does.

@MartinJohns

To be fair, I think the entire data structure you propose is undesirable. It's unsound and can lead to hard-to-find bugs.

That's entirely valid, but to be clear my choice of any was misleading and just an artifact of some of the code I'm dealing with at the moment. (Specifically there are limitations around OpenAPI client generation, but that's irrelevant)

type Foo = {
  bar: string;
  baz: number;
} & Record<string, number | string>

type OmitFoo = Omit<Foo, "baz">

Still demonstrates what I believe to be undesirable behaviour, and isn't unsound (if I understand your critique correctly)

@MartinJohns
Copy link
Contributor

That's entirely valid, but to be clear my choice of any was misleading and just an artifact of some of the code I'm dealing with at the moment.

Changes nothing, really. As soon as you have a union type you can end up with unsound behaviour.

@luckylooke
Copy link

Imho every human reading that code in issue description would expect same result as the one asked, is pretty intuitive and there is no ambiguity in it. It should omit from known members and other kept untouched.

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

No branches or pull requests

4 participants