-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Discussion on --strictOptionalProperties
#44421
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
Comments
New Errors list from last night #44427 👀 |
Last night I tried updating pyright/pylance to a build of TS that included this to see how it would go. I'll try and sum up my thoughts... I'm not sure that this check is going to end up being widely usable any time soon; I get loads of messages coming from the types of libraries I don't control, especially from the LSP client library (and spec), where they treat As for trying to fix our own code, the messaging is definitely bad; I found it very hard to try and locate the actual place I needed to make the change in a large project. I ended up starting tsc in watch mode, opening a file with an error in VS Code, then I see the problems (as VSC only shows errors in open files). Then, either the related diagnostic pointed back to the declaration (so I could jump and add I'm sure it could be improved to say something like "property had a Before I gave up, I still had hundreds of errors to fix. It seemed almost random which properties I needed to change, and I was left with a number of interfaces where some properties had I'm not sure I've actually been able to find any bugs with this so far; maybe I'm too inclined to just try and stick I'm pretty surprised that this is on by default; it feels really noisy (far noisier than the not-default |
Is the I thought that it was supposed to just remove the |
I believe language features should be a right balance between exactness and usability, i.e. a health dose of pragmatism. Now, because I use the I guess the issue here stems from the fact that there are many levels of strictness, and because |
There are - set the individual flags you need. Multiple |
Fun fact: When I used to write plain JS, I did |
It does, you can create a type like
Which I also believe would be true for most third party libraries as well. If the new |
const oops: { foo: string } | {} = { foo: undefined }; // no error Excess property checks only happen in specific situations, and they never happen for the empty object type
I guess "widely" is a subjective term, but #13195 does have a decent number of positive reactions, comments, and linked issues. |
|
For the record, while that approach may work for small numbers of optional properties (3-4, tops), it would become combinatorially explosive very quickly. Also, the |
So |
For context, if we're looking at type system features, this was easily in the top 10 in terms of upvotes (it's behind only typed exception clauses, parametric types, nominal types, exact types, spread types, type guard inference, and I definitely hear the feedback, though -- some codebases consistently disavow any difference between There are many APIs in JS that don't work like this, though (including the spread operator |
The spread operator was definitely what did it for me. It was always painful to know that I couldn't trust the type system to back me up on |
On the one hand I agree, that having this new strictness would be very helpful. I just ran into problems using lodash/isEqual because some nested fields sometimes existed as undefined and sometimes not. On the other hand there are object literals, where there is no good syntax for ignoring undefined entries. const combined = {
propertyOne: this.someService.getAValueMaybe(),
anotherProperty: this.whereever?.nested?.deeply?.list?.length,
} Cf. #39376 What about adding as a warning first (not sure how, @typescript-eslint?) and only in v5 making --strictOptionalProperties the default setting? |
We spoke about this at our last design meeting (#44524). We've decided:
|
Would you be able to address this question? I hope I'm not completely misunderstanding things. |
Aren't there already several other flags prefixed with |
@glen-84 I don't want to paraphrase the intent - @ahejlsberg can probably do an explanation better justice. |
@fatcerberus I don't think so. I checked by searching |
If I'm trying to think of a case where you'd want an object with all its properties set to {
firstName?: string | undefined, // Missing, a string, or undefined.
lastName?: string | undefined, // Missing, a string, or undefined.
age?: number | null | undefined; // Missing, a number, null, or undefined.
} When would you use something like this, mixing regular data with explicit Don't most people use {
firstName?: string, // Missing, or a string.
lastName?: string, // Missing, or a string.
age?: number | null; // Missing, a number, or null.
} i.e. Allow me to omit certain properties, not allow me to set certain properties to
I'm probably missing something obvious, so forgive me. This relates to my question about |
The notes didn't quite capture it, but our conclusion was that |
Please post new name ideas at #44604 |
What would be the best option to make this working properly? interface Foo { optional?: string }
function foo (value: Foo): Foo {
return {
optional: value.optional,
};
} Currently there's no problem with that, since return 'optional' in value ? {
optional: value.optional,
} : {}; which would be a big pain for more advanced usecases. My first thought would be to introduce a function foo (value: Foo): Foo {
return {
optional?: value.optional
};
}
foo({}); // returns {}
foo({ optional: '' }); // returns { optional: '' } but i'm pretty sure it would be quickly rejected for various reasons. So is checking |
@jacekkarczmarczyk I'd write a standalone |
Literal object syntax is very useful and my concern as well with the new option. |
I tried the new option on our codebase and I think it is a step in the right direction:
The same problem described by @jacekkarczmarczyk also directly translates to JSX, and some ways to solve it: const noop = () => {}
const Buttons = ({
onFoo = noop,
onBar,
...props,
} : {
onFoo?: () => void,
onBar?: () => void,
onBlub: () => void,
onClick?: () => void,
}) =>
{
return <div>
<button onClick={onFoo}>Do Foo</button>
<button onClick={onBar ?? noop}>Do Bar</button>
<button onClick={onBlub ?? noop}>Do Blub</button>
<button {...props}>Click Me</button>
</div>
}
const root = <Buttons onBlub={noop} /> |
How about use interface Foo {
prop: string | void;
} By the way, in void fn1() {}
void fn2(void) {}
fn1(1); // OK
fn2(1); // Error |
I very much agree with this:
This feels like a real step backwards in terms of readability, and I, too, have rarely if ever come across a spot where I cared about the difference between a missing property and a property whose value is And sure, I can turn the flag off for my code, but I maintain one of those third party libraries discussed earlier, and so if I don't want to break my users who have this turned on, IIUC I more or less have to turn it on myself, or act like it's turned on, at least, right? If so, I really don't like that this has been forced on us. Is there any way to add a flag to say, "Not only do I not care about the missing/undefined distinction in my own code, I want consumers of my code to be fine with it, too"? If my code doesn't act any differently based on the missing/undefined distinction, why should it matter which one an |
@DanielRosenwasser @RyanCavanaugh - Pinging on this. Any chance you'd consider the above? (Or perhaps I'm wrong about this?
) |
Every strictness flag we've added since day 1 has implied work for upstream maintainers. We assessed the impact of this on DefinitelyTyped and other code bases and felt it was a worthwhile trade-off. I'm not sure what sort of behavior you're describing with the suggestion. A concrete proposal of what specifically you think could work differently would be a good next step. |
Sorry for not being clear. What I mean is this:
But my understanding is that if anyone using What I'd like is not to have to do that, because IMHO it makes our types cluttered and harder to read, all in order to preserve a distinction our code doesn't make. So I wish there were a setting we could set along the lines of Hopefully that makes more sense. Failing such an option, perhaps there could be a new Thanks. |
Just my 2 cents. I'm starting with typescript and stumbled upon this issue when trying to use this flag. Here's my simple function: async function health(signal?: AbortSignal): Promise<void> {
const response = await fetch(`${backendUrl}/health`, {
signal,
});
const text = await response.text();
if (!response.ok) {
throw new Error(
"Failed to check health: " +
`${response.status} ${response.statusText}: ${text}`,
);
}
} I didn't find any ergonomic way to implement this code with new flag. Here's one way I've come with but it's ugly: async function health(signal?: AbortSignal): Promise<void> {
const response = await fetch(`${backendUrl}/health`, {
...(signal !== undefined ? { signal } : {}),
});
const text = await response.text();
if (!response.ok) {
throw new Error(
"Failed to check health: " +
`${response.status} ${response.statusText}: ${text}`,
);
}
} I think there should be a language feature to support this compiler flag to allow for more ergonomic usage. In this particular case with AbortSignal, there's actually another way: That said, I like this flag because it helped me to spot some weird API I designed (like using |
@vbezhenar I don't understand what's wrong with the upper snippet in your comment. It maybe sounds like what you're saying is that you want a way to construct an object literal that conditionally has a property slot? |
@RyanCavanaugh it does not compile with exactOptionalPropertyTypes. Here's some declarations from lib.dom.d.ts:
So with exactOptionalPropertyTypes enabled I can't pass One work-around is to use second snippet: add property conditionally. Another work-around is to pass So, yes, a way to construct object that conditionally has a property slot with more ergonomic syntax would help. I think someone already suggested syntax like:
Now I understand that TypeScript's philosophy is not to introduce syntax which deviates from JavaScript so I guess it's better to direct that kind of feedback to JavaScript standardization committee. |
This has been mentioned above already, but in case you missed it, #39376 is the relevant typescript issue which was rejected and here is the tc39 discussion: https://es.discourse.group/t/optional-property-shorthand-in-object-literal/196 |
For working with exactOptionalPropertyTypes code base I found this utility function to be super useful: /**
* Removes all undefined properties from an object.
*/
export function omitUndef<O extends object | undefined>(object: O):
& { [K in keyof O as undefined extends O[K] ? never : K]: O[K] }
& { [K in keyof O as undefined extends O[K] ? K : never]?: O[K] & ({} | null) }
{
if (object == undefined) return object as never;
return Object.fromEntries(
Object.entries(object).filter(([, v]) => v !== undefined),
) as never;
} |
We've pulled in a new feature called
--strictOptionalProperties
which sits under the--strict
family of flags. If we were to start TypeScript over again, we believe the behavior ofstrictOptionalProperties
would be on by default; however, this strictness option comes at a time when the community is much more mature with lots of existing code.I've opened up this issue to discuss some broader concerns so that we can ensure that
--strictOptionalProperties
ships smoothly and so we can hear some feedback from users.Off the bat, here's a few of my own concerns:
Discoverability of the Break
A user who upgrades TypeScript with
--strict
on currently has no idea why their code has broken. At best, they will get an error likeWe can do better here, which is why I've opened up #44403.
Manual Upgrade Woes
The most permissive thing to do to fix breaks from
--strictOptionalProperties
is to tack on an| undefined
onto every single?
-marked optional property; however, this is incredibly tedious for an existing project.We'd like to provide some automated tooling to help alleviate this, which is why I've opened up #44419.
Surrounding Community Upgrade Woes
There are really 3 places that are outside most users' control that need to be upgraded to work correctly with
strictOptionalProperties
:lib.d.ts
@types
packages)The last one is the hardest to solve; however, the first two are way more automatable, and we can have the core team help with it, but it's definitely time taken away from other work, and that's a delicate tradeoff.
I haven't created an issue to manage this one, but I think it might be worth distributing segments of
lib.d.ts
and some number of top packages to add| undefined
to the definitions and see if that helps. Ideally, automation from #44419 could help. We'd also have to update thelib.dom.d.ts
generator to emit| undefined
on most optional properties.I'd like to get @RyanCavanaugh's thoughts on this one.
Other Feedback
I'm curious to hear feedback from users on this one - is it a useful option? Do you feel good about it and its purpose? Did it catch any interesting bugs? Let us know!
The text was updated successfully, but these errors were encountered: