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

Please consider enabling exactOptionalPropertyTypes by default #60636

Open
6 tasks done
allisonkarlitskaya opened this issue Nov 28, 2024 · 4 comments
Open
6 tasks done
Labels
Declined The issue was declined as something which matches the TypeScript vision Suggestion An idea for TypeScript

Comments

@allisonkarlitskaya
Copy link

🔍 Search Terms

exactOptionalPropertyTypes in the subject line on open and closed issues and PRs in this repository

✅ Viability Checklist

⭐ Suggestion

exactOptionalPropertyTypes is great, and it's been around for a few years by now. It's a recommended option, and having it off means that | undefined gets implicitly sprinkled around on your types.

I'd like to see this feature enabled by default. It's a clear improvement to the semantics of the language and (with the benefit of hindsight) it probably should have been the way this was done in the first place.

I understand that turning this option on is going to break a lot of existing code (see the above unchecked box edit okay, it forced me to check it), and that this is why it hasn't been done yet. I wonder if the default could be changed to a special mode that's compatible with the old default value (false) but also avoids generating errors caused by undefined not being compatible with other types. See the example below for the specific case where we're getting in trouble.

In general, though, it would be awesome if there was a mechanism to allow advancing "new defaults" for language features, something akin to Rust "editions". That's a way larger question, though.

📃 Motivating Example

The main issue is that if you have this feature on in your codebase you can produce APIs that don't work when people try to import/vendor them into their codebase with that option disabled.

We recently got this bug report: cockpit-project/cockpit#21352

In short, we have a "json" type defined in a rather typical way:

export type JsonValue = null | boolean | number | string | JsonValue[] | { [key: string]: JsonValue };
export type JsonObject = Record<string, JsonValue>;

but then we sometimes say things like

export interface BaseChannelOptions extends JsonObject {
    command?: never;
    channel?: never;
    binary?: boolean;
    host?: string;
    payload?: string;
    superuser?: "try" | "require";
}

to produce an interface which is required to never have the properties command or channel present.

This works great with exactOptionalPropertyTypes but if our code gets used by someone who doesn't have that option set they see an error like:

pkg/lib/cockpit/channel.ts:31:3 - error TS2411: Property 'command' of type 'undefined' is not assignable to 'string' index type 'JsonValue'.

💻 Use Cases

See above.

Thanks very much!

@MartinJohns
Copy link
Contributor

In general, though, it would be awesome if there was a mechanism to allow advancing "new defaults" for language features

#56028

Not a team member, but I can tell you that changing the default is not going to happen. There was even plenty of discussion whether this should be part of strict, and they decided against it.

@RyanCavanaugh RyanCavanaugh added Suggestion An idea for TypeScript Declined The issue was declined as something which matches the TypeScript vision labels Dec 2, 2024
@RyanCavanaugh
Copy link
Member

This was considered at the time we added the feature and there don't appear to be any different facts on the ground than there were then. It'll likely be "soft-recommended" in the next update to tsc --init (listed as a commented-out "you might want this" option).

@ritschwumm
Copy link

@RyanCavanaugh would be interesting to know how many projects have it enabled by now - mine certainly all have. if it's a largeish fraction then probably not too many people would be annoyed if it became the default.

@allisonkarlitskaya
Copy link
Author

allisonkarlitskaya commented Dec 2, 2024

This was considered at the time we added the feature and there don't appear to be any different facts on the ground than there were then. It'll likely be "soft-recommended" in the next update to tsc --init (listed as a commented-out "you might want this" option).

I agree that this isn't the sort of thing that should just be turned off overnight - the summary I chose for the issue was perhaps a bit too provocative. I mostly wanted to start a conversation about the issues that a situation like this causes and come up with ideas for how to resolve this situation.

I think the gist of this issue is that this option has been 'soft recommended' for some time (in the documentation here, marked "Recommended"). People who are aware of that are using it, and they're happy with it, but they're effectively writing a different version of the language (let's call it "Recommended") that is different from the "Default" version of the language being written by people who don't know this option.

Those two language versions are by and large the same language, but there are some incompatibilities if you fall into an edge case.

The result of this is that our downstream gets stuck an impossible situation because:

And indeed, that's exactly the reason why this feature can't be considered as a "strict" feature in the same sense as the other "strict" options: it's incompatible.

So, I think a lot of this is an awareness thing. The OpenAPI bug is sitting open for quite some time. If there were some stronger signals from the TypeScript side along the lines of "no, really, we definitely want you to enable this", then it would help a lot to (let's say) "show the direction". Because right now, I think it kinda seems like it's sort of a matter-of-taste/opinion sort of question.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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

4 participants