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 outputting incorrect types for types with mixed [key: string]: string | undefined and defined field types #49656

Closed
james-reed-toshi opened this issue Jun 23, 2022 · 14 comments
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@james-reed-toshi
Copy link

james-reed-toshi commented Jun 23, 2022

Bug Report

Using Omit and [key: string]: string | undefined together can result in Typescript not preserving the types of fields on the original type. Playground link here to display the issue: playground link for issue here

The issue is that the fields that were previously defined as strings, are now returning as string | undefined, even though there's no reason they should, type-wise. If you remove the [key: string]: string | undefined the problem goes away, and typescript is obviously clever enough to know that the fields line1, line2 and postcode are just of type string, not string | undefined.

🔎 Search Terms

omit, omit undefined

🕗 Version & Regression Information

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

⏯ Playground Link

Link given above

💻 Code

interface Address {
    [key: string]: string | undefined;
    line1: string;
    line2: string;
    postcode: string
}

interface Parent {
    id: string;
    party: boolean;
    address: Omit<Address, 'line2'>;
}

const parentFirst: Parent = {
    id: '',
    party: false,
    address: {
        line1: '',
        postcode: '',
    },
};

const finalAddress: Address = {
    line1: parentFirst.address.line1,
    postcode: parentFirst.address.postcode,
    line2: '',
}
  
// (property) Address.line1: string
// Type 'string | undefined' is not assignable to type 'string'.
//   Type 'undefined' is not assignable to type 'string'.(2322)

// Same error happens for poscode

🙁 Actual behavior

Omit does not preserve the types of the named fields

🙂 Expected behavior

Omit should preserve the types of the named fields

@MartinJohns
Copy link
Contributor

#45367 (comment)

@james-reed-toshi
Copy link
Author

Oh right, ok. Would adding this

type FieldTypePreservingOmit<T, K extends keyof any> = { [P in keyof T as Exclude<P, K>]: T[P] };

as a type in my codebase be the best course of action in this case? It seems to preserve the type of the fields. In fact, if this preserves the types of the fields, why is this not how Omit is coded? Would be interested to know why this is desired behaviour.

@RyanCavanaugh
Copy link
Member

why is this not how Omit is coded

Omit is a type alias for a thing whose code you can read; it isn't a higher-order operation that understands what it's doing at a higher level.

type Omit<T, K extends keyof any> = Pick<T, Exclude<keyof T, K>>;

It's generally pretty hard to work with types that have index signatures along with their declared types; I would rewrite this as

interface Address {
    line1: string;
    line2: string;
    postcode: string
}

interface Parent {
    id: string;
    party: boolean;
    address: Omit<Address, 'line2'> & { [key: string]: string | undefined };
}

@RyanCavanaugh RyanCavanaugh added the Working as Intended The behavior described is the intended behavior; this is not a bug label Jun 23, 2022
@james-reed-toshi
Copy link
Author

@RyanCavanaugh This is unfortunately a more legacy codebase and I can't remove the index signature without a lot of work elsewhere.

@james-reed-toshi
Copy link
Author

james-reed-toshi commented Jun 23, 2022

Omit is a type alias for a thing whose code you can read; it isn't a higher-order operation that understands what it's doing at a higher level.

I understand that, but if it can do that and do better for the user, what's the downside with defining it as I did above with FieldTypePreservingOmit?

edit: Please understand I'm asking this from a position of ignorance, not a position of 'I think I could do better' :) dont want you thinking just because I've put an issue in that I know better than the entire TS team 😂

@RyanCavanaugh
Copy link
Member

Ah, good question.

FieldTypePreservingOmit uses the key remapping feature that didn't exist when Omit was written, and changing the definition of Omit after the fact would be a breaking change. One of the reasons we don't like adding type aliases to the built-in lib -- we can't "upgrade" them!

@JamesZoft
Copy link

Aha, that makes sense! Is there a way, then, that I can propose this as an upgrade to Omit for v5 @RyanCavanaugh ?

@RyanCavanaugh
Copy link
Member

We don't issue large breaking changes, regardless of version numbers, unless there's a very identifiable concrete upside. I don't think tweaking Omit for this scenario would meet that bar.

@JamesZoft
Copy link

JamesZoft commented Jun 23, 2022 via email

@RyanCavanaugh
Copy link
Member

--noLib + provide your own copies of the lib files

@JamesZoft
Copy link

JamesZoft commented Jun 23, 2022 via email

@RyanCavanaugh
Copy link
Member

You can lexically shadow it (including at module scope), but you can't globally clobber it

@JamesZoft
Copy link

JamesZoft commented Jun 24, 2022 via email

@typescript-bot
Copy link
Collaborator

This issue has been marked 'Working as Intended' and has seen no recent activity. It has been automatically closed for house-keeping purposes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests

5 participants