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

Discussion on --strictOptionalProperties #44421

Closed
DanielRosenwasser opened this issue Jun 3, 2021 · 36 comments
Closed

Discussion on --strictOptionalProperties #44421

DanielRosenwasser opened this issue Jun 3, 2021 · 36 comments
Labels
Discussion Issues which may not have code impact Meta-Issue An issue about the team, or the direction of TypeScript

Comments

@DanielRosenwasser
Copy link
Member

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 of strictOptionalProperties 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 like

Type 'T | U | V | undefined' is not assignable to type 'T | U | V'.
  Type 'undefined' is not assignable to type 'T | U | V'.

We 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
  • DefinitelyTyped (@types packages)
  • Libraries shipping their own types

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 the lib.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!

@DanielRosenwasser
Copy link
Member Author

DanielRosenwasser commented Jun 4, 2021

New Errors list from last night #44427 👀

@jakebailey
Copy link
Member

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 equivalent to undefined. I think that this pattern is very common and unlikely go away.

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 | undefined), or confusingly did not, in which case I had to go-to-def on the error'd expression to figure out what to change.

I'm sure it could be improved to say something like "property had a ? but no undefined", but that is going to push everyone to add | undefined, in which case, is this check helping?

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 | undefined and some did not, which tells me that at some point in the future, I may use a property in a new way and have to fix them.

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 | undefined everywhere. I don't think we do any in checks, and only care about truthiness.

I'm pretty surprised that this is on by default; it feels really noisy (far noisier than the not-default noPropertyAccessFromIndexSignature) and I expect to have to disable it when I upgrade. I think it will be very surprising to users with "strict": true to see this enabled.

@glen-84
Copy link

glen-84 commented Jun 4, 2021

Is the Required utility type supposed to be removing an explicit undefined?

https://www.typescriptlang.org/play?strictOptionalProperties=true&strict=true&ts=4.4.0-dev.20210602#code/JYOwLgpgTgZghgYwgAgKIA84FsAOAbFAbwChkzk4B+ALmQGcwpQBzZAH2QFcQATCGUBB4BuYgF9ixMAE8cKAILIAvMgAKwBAGsAPBmz4IAGmQAiOCYB8omXOTyASsuT2IAR07AoQ7fKuSEAPYgDMjotA5OhHC03HwCIEJiokA

I thought that it was supposed to just remove the ?.

@OoDeLally
Copy link

OoDeLally commented Jun 6, 2021

I believe language features should be a right balance between exactness and usability, i.e. a health dose of pragmatism.
With this strictOptionalProperties, I feel like we are sacrificing a lot of usability and readability, just for the purpose of exactness.
From my experience there are very little cases (in fact, I've never encountered any) where I care about whether a field is undefined or missing (and if I ever did, there are ways to assert that).

Now, because I use the strict option, I have to clutter all my types with | undefined.

I guess the issue here stems from the fact that there are many levels of strictness, and because strict is binary, the true value means all possible strictness rules. Maybe there could be several levels.

@fatcerberus
Copy link

Maybe there could be several levels.

There are - set the individual flags you need. Multiple strict levels + individual options would just be way too much cognitive overhead IMO. If I'm at the point where I'm asking myself "Which level of strictness do I need?" I should probably be researching what exactly my options are anyway.

@fatcerberus
Copy link

From my experience there are very little cases (in fact, I've never encountered any) where I care about whether a field is undefined or missing (and if I ever did, there are ways to assert that).

Fun fact: When I used to write plain JS, I did "foo" in obj tests all the time. TS forced me out of that habit (very begrudgingly) because it had, until recently, no type-level way to distinguish between ? and | undefined. So while I consider myself pragmatic, I'm hesitant to throw around pragmatism as an excuse here because I never considered the lack of strictOptionalProperties to be very pragmatic 😉

@Pajn
Copy link

Pajn commented Jun 7, 2021

Fun fact: When I used to write plain JS, I did "foo" in obj tests all the time. TS forced me out of that habit (very begrudgingly) because it had, until recently, no type-level way to distinguish between ? and | undefined.

It does, you can create a type like {foo: string} | {} if you want foo to be optional but not allow undefined as a value so the question is more about final type is more desirable and thus should be represented with the most ergonomic {foo?: string} syntax. Personally I beleave the old way is more common partly because of this statement in the initial post:

We'd also have to update the lib.dom.d.ts generator to emit | undefined on most optional properties.

Which I also believe would be true for most third party libraries as well. If the new strictOptionalProperties behaviour was more common it would have been a widely requested feature up until now. However, now that I know about it I can just add strictOptionalProperties: false to all my tsconfigs so it's not a massive problem. Though if library authors start adding strictOptionalProperties: false they would not catch this and release typings that causes problems for strictOptionalProperties: true so there is a small risk that this causes a lot of churn for both users and library authors.

@jcalz
Copy link
Contributor

jcalz commented Jun 7, 2021

It does, you can create a type like {foo: string} | {} if you want foo to be optional but not allow undefined as a value

Playground link

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 {}. There aren't any truly exact types (#12936) so even with excess property checking you could find fun ways to sneak undefined (or even unknown) property values in there.

it would have been a widely requested feature up until now

I guess "widely" is a subjective term, but #13195 does have a decent number of positive reactions, comments, and linked issues.

@Pajn
Copy link

Pajn commented Jun 7, 2021

{ foo: string } | { foo: never } then, it's always possible to type it if you actually need it. But I'm not arguing against the existence of this, as I said, I'll just disable it and move on.

@fatcerberus
Copy link

fatcerberus commented Jun 7, 2021

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 never may become an issue in the presence of other properties. By definition, I can't produce a value of type { foo: string, bar: never }

@jcalz
Copy link
Contributor

jcalz commented Jun 7, 2021

{ foo: string } | { foo: never }

So foo is a required property, I guess? If this were a straightforward problem to solve by using existing language features, then it would have been done that way instead of by introducing a new compiler flag.

@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented Jun 7, 2021

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 internal).

I definitely hear the feedback, though -- some codebases consistently disavow any difference between { } and { k: undefined }. If that's true in your codebase, turning this flag on is all pain and no gain.

There are many APIs in JS that don't work like this, though (including the spread operator ...), and faithful typing of real JS API is our north star. I think it was truly a "missing" (ha) feature and the question is just how to get it out there in a way that isn't too painful, even if "too painful" means "too much to toggle a flag in your tsconfig".

@fatcerberus
Copy link

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 { ...defaults, ...userValues } - the exact use case I had in mind when I first learned the spread operator was going to be a thing in JS.

@bodograumann
Copy link

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.
E.g.

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?

@DanielRosenwasser
Copy link
Member Author

DanielRosenwasser commented Jun 10, 2021

We spoke about this at our last design meeting (#44524). We've decided:

  • We will be moving this flag out of --strict. We believe the flag is too invasive, even for --strict.
  • We will be keeping the flag around so that users can opt into this new mode (like noUncheckedIndexedAccess). We believe that the flag still has utility.
  • We will be renaming the flag, since prefixing it with strict would be misleading.
  • We still want to make the ecosystem ready (e.g. updating lib.d.ts, Definitely Typed).
  • We want to include the flag in --init once the ecosystem is ready.

@glen-84
Copy link

glen-84 commented Jun 10, 2021

@DanielRosenwasser,

Would you be able to address this question? I hope I'm not completely misunderstanding things.

@fatcerberus
Copy link

We will be renaming the flag, since prefixing it with strict would be misleading.

Aren't there already several other flags prefixed with strict that don't get enabled with strict: true?

@DanielRosenwasser
Copy link
Member Author

@glen-84 I don't want to paraphrase the intent - @ahejlsberg can probably do an explanation better justice.

@DanielRosenwasser
Copy link
Member Author

DanielRosenwasser commented Jun 10, 2021

@fatcerberus I don't think so. I checked by searching "strict on http://json.schemastore.org/tsconfig and didn't find anything surprising, but I'm about to jump into a meeting. You can also check https://github1s.com/microsoft/TypeScript/blob/v4.3.2/src/compiler/commandLineParser.ts

@glen-84
Copy link

glen-84 commented Jun 10, 2021

Partial? Should it include | undefined?

If Partial includes undefined under this mode, then surely there's still no way of saying "any property may be missing, but if it's present, its type is X (which might not include undefined)"?

I'm trying to think of a case where you'd want an object with all its properties set to | undefined.

{
	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 undefineds?

Don't most people use Partial so that they can work with part of an object?

{
	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 undefined.

undefined is still included when reading from the property, as it could be missing (matching the runtime behaviour).

I'm probably missing something obvious, so forgive me.

This relates to my question about Required, which is effectively the opposite. I thought that under this mode, it would simply remove the possibility of the property being missing, but if the property was typed to include undefined, then that would be left as-is.

@RyanCavanaugh
Copy link
Member

The notes didn't quite capture it, but our conclusion was that Partial shouldn't add undefined, because its primary use case is in APIS like setState where present-vs-missing is an important distinction

@RyanCavanaugh
Copy link
Member

Please post new name ideas at #44604

@jacekkarczmarczyk
Copy link

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 optional?: string can contain a property with undefined value, however with strictOptionalProperties flag you would need to do

  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 property?: value, working as follows

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 'optional' in value the only way to go?

@RyanCavanaugh
Copy link
Member

@jacekkarczmarczyk I'd write a standalone copy(obj, list of keys) function (this is what I'd do even in untyped JS)

@bodograumann
Copy link

Literal object syntax is very useful and my concern as well with the new option.
The new syntax you propose, @jacekkarczmarczyk, would be my preferred solution,
but it seems it is out-of-scope for typescript. Discussion: #39376
In javascript itself getting it in is a much harder task and I’m not sure it will ever happen.

@pyBlob
Copy link
Contributor

pyBlob commented Jun 25, 2021

I tried the new option on our codebase and I think it is a step in the right direction:

  • It actually has discovered one or two problems related to checking/overwriting undefined properties
  • It finally makes type guards related to checking for optional properties be more precise (which is in line with the original goal).
  • Though you can clearly see that the ecosystem of libraries still need to get on board to permanently enable the option.

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} />

@maroon1
Copy link

maroon1 commented Aug 11, 2021

How about use void to exactly indicate a property is missing.

interface Foo {
  prop: string | void;
}

By the way, in C or C++, if some function never receive parameters, it can declare argument as void

void fn1() {}

void fn2(void) {}

fn1(1); // OK
fn2(1); // Error

@DanielRosenwasser DanielRosenwasser unpinned this issue Aug 11, 2021
@lobsterkatie
Copy link

lobsterkatie commented Nov 8, 2021

I very much agree with this:

I believe language features should be a right balance between exactness and usability, i.e. a health dose of pragmatism.
With this strictOptionalProperties, I feel like we are sacrificing a lot of usability and readability, just for the purpose of exactness.
From my experience there are very little cases (in fact, I've never encountered any) where I care about whether a field is undefined or missing (and if I ever did, there are ways to assert that).

Now, because I use the strict option, I have to clutter all my types with | undefined.

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 undefined. Instead of making it so that code which doesn't care (which I contend is most code) has to specifically opt out of caring, it'd would've been great if code which does care had to opt in to caring.

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 exactOptionalPropertyTypes-using app passes me?

@lobsterkatie
Copy link

lobsterkatie commented Dec 10, 2021

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 exactOptionalPropertyTypes-using app passes me?

@DanielRosenwasser @RyanCavanaugh - Pinging on this. Any chance you'd consider the above?

(Or perhaps I'm wrong about this?

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?

)

@RyanCavanaugh
Copy link
Member

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.

@lobsterkatie
Copy link

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:

@sentry/browser doesn't make the distinction between missing values and values set to undefined. If we were only building for ourselves, we'd set exactOptionalPropertyTypes to false and continue to just use optional properties in our types, and all would be well.

But my understanding is that if anyone using @sentry/browser has exactOptionalPropertyTypes set to true, and they pass undefined in for some value we've marked as optional, they'll get an error. Currently, the only way for us to help such a user is to add | undefined to all of our optional properties (or at least, any which might conceivably get passed undefined). Correct?

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 pretendAllOptionalPropertiesInAnyTypesWeExportHaveAnOrUndefinedOnTheEnd. In other words, I'd like to be able to give users of @sentry/browser blanket permission to pass undefined in for any optional property in our types, even if those users have exactOptionalPropertyTypes set to true.

Hopefully that makes more sense. Failing such an option, perhaps there could be a new ?? designation for properties, so that someProp?: string would mean "pass a string or nothing at all" and someProp??: string would mean "pass a string or undefined or nothing at all" (with ?? essentially playing the role ? used to play). Quite frankly I'd take anything which would make this work in a way which doesn't sacrifice so much readability in order to solve a problem which (as far as usage of our SDK is concerned) doesn't exist.

Thanks.

@vbezhenar
Copy link

vbezhenar commented Feb 14, 2023

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: signal: signal ?? null because RequestInit declares signal as signal?: AbortSignal | null;. But I think that API like that is not always available.

That said, I like this flag because it helped me to spot some weird API I designed (like using x?: X instead of x: X | undefined.

@RyanCavanaugh
Copy link
Member

@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?

@vbezhenar
Copy link

@RyanCavanaugh it does not compile with exactOptionalPropertyTypes.

Here's some declarations from lib.dom.d.ts:

declare function fetch(input: RequestInfo | URL, init?: RequestInit): Promise<Response>;

interface RequestInit {
    ...
    signal?: AbortSignal | null;
}

So with exactOptionalPropertyTypes enabled I can't pass undefined to signal.

One work-around is to use second snippet: add property conditionally.

Another work-around is to pass null because this particular API allows to pass null, but not every API allows that.

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:

fetch(url, { signal?: signal })

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.

@bodograumann
Copy link

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

@phaux
Copy link

phaux commented Oct 19, 2023

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;
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion Issues which may not have code impact Meta-Issue An issue about the team, or the direction of TypeScript
Projects
None yet
Development

No branches or pull requests