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

Object.values removes undefined from optional properties #48587

Closed
WiseBird opened this issue Apr 6, 2022 · 9 comments
Closed

Object.values removes undefined from optional properties #48587

WiseBird opened this issue Apr 6, 2022 · 9 comments
Labels
Question An issue which isn't directly actionable in code

Comments

@WiseBird
Copy link

WiseBird commented Apr 6, 2022

Bug Report

πŸ”Ž Search Terms

enum index signature values
object values infer optional

πŸ•— Version & Regression Information

  • This changed between versions 4.2.3 and 4.3.5

⏯ Playground Link

Playground link with relevant code

πŸ’» Code

In the following code type of values returned by Object.values is correctly infered to string | undefined:

type Type = {
    one: string | undefined;
    two: string | undefined;
}

const typeMap: Type = {} as any;
typeMap.one = undefined;

for(const value of Object.values(typeMap)) {
    console.log(value.length); // Object is possibly 'undefined'
}

But if I add ? to the properties, the type is now string

type TypeOptional = {
    one?: string | undefined;
    two?: string | undefined;
}

const typeOptMap: TypeOptional = {} as any;
typeOptMap.one = undefined;

for(const value of Object.values(typeOptMap)) {
    console.log(value.length); // no error here
}

Real-world example is about using enums for indexing an object:

enum Enum {
    one = 'one',
    two = 'two',
}

const enumMap: {[inex in Enum]?: string} = {};
enumMap[Enum.one] = undefined;

for(const value of Object.values(enumMap)) {
    console.log(value.length); // no error here as well
}

πŸ™ Actual behavior

Values from properties with ? and | undefined are considered non-nullish.

πŸ™‚ Expected behavior

Error in all the cases

@whzx5byb
Copy link

whzx5byb commented Apr 6, 2022

The option exactOptionalPropertyTypes will produce your expect behavior.

@WiseBird
Copy link
Author

WiseBird commented Apr 6, 2022

True, but the error should be reported even if exactOptionalPropertyTypes is not set

@IllusionMH
Copy link
Contributor

TS doesn't differentiate between key?: type and key?: type | undefined without exactOptionalPropertyTypes. That's the whole point of the flag.

See #13195

@RyanCavanaugh RyanCavanaugh added the Question An issue which isn't directly actionable in code label Apr 7, 2022
@WiseBird
Copy link
Author

WiseBird commented Apr 9, 2022

@IllusionMH
In case when exactOptionalPropertyTypes is off and TS can't differentiate between key?: type and key?: type | undefined, shouldn't it assume that values can be undefined?
In my example TS does it but partially:

type TypeOptional = {
    one?: string | undefined;
    two?: string | undefined;
}

const typeOptMap: TypeOptional = {};
typeOptMap.one = undefined; // Here it's fine to assign `undefined`

for(const value of Object.values(typeOptMap)) {
    console.log(value.length); // Here no error accessing the length as if value couldn't be undefined
}

@IllusionMH
Copy link
Contributor

That's how it was implemented initially and changing that would break a lot of existing code.

But for cases like yours new flag was implemented so you can enable it and see expected errors.

What's the problem with flag? Just enable it ang get errors in your code base without breaking code of others.

@WiseBird
Copy link
Author

@IllusionMH
I don't have anything against the flag. I just feel that TS can be more restrictive here.
Consider this slightly changed example:

type TypeOptional = {
    one?: string | undefined;
    two?: string | undefined;
}

const typeOptMap: TypeOptional = {};
'one' in typeOptMap && console.log(typeOptMap.one.length); // Object is possibly 'undefined'.

for(const value of Object.values(typeOptMap)) {
    console.log(value.length); // no errors
}

TS knows that the value is nullable when you directly access it, but not when you get it from Object.values.
If changing this will break a lot of existing code then I guess better leave it as it is.

@typescript-bot
Copy link
Collaborator

This issue has been marked as 'Question' and has seen no recent activity. It has been automatically closed for house-keeping purposes. If you're still waiting on a response, questions are usually better suited to stackoverflow or the TypeScript Discord community.

@eliasm307
Copy link

Is there any possibility of having this show a warning by default atleast?

I'm working in an existing codebase where enabling exactOptionalPropertyTypes would break a lot of things. I think there is room for improvement if the only options for this are to either:

  • live with TS allowing potentially buggy code when iterating object property values
  • enable a feature that means we can't assign undefined to optional properties at all (which some people might not want) and potentially have to refractor code.

TS is meant to allow us to "Catch errors early in your editor" which it does excellently but in this case its surprising the only way to get any indication of an issue is to make a potentially major change with some refactoring cost.

I haven't tried it but I guess enabling exactOptionalPropertyTypes and making the type of all optional properties unions with undefined could maintain existing behaviour with the flag on but not sure if it would show undefined when iterating over object values πŸ€” I'll give it a go when I'm on a computer next, however if it does work it would make the types look a bit odd and some refactoring cost is still required.

Also @IllusionMH you mentioned "that's how it was implemented initially" however it seems this behaviour changed in v4.3, where it used to show undefined values when an object had optional properties, so is this not a regression?

@d07RiV
Copy link

d07RiV commented Dec 7, 2023

Would it be possible to allow x?: string | undefined to work without the flag? As mentioned, the flag can break a lot of existing code (since it prevents assigning undefined to optional fields), which is why it's not on by default, but there is no reason why someone would write string | undefined on optional types if they didn't want to specifically imply that the value can be present but undefined.

Specifically, the flag is very problematic to use in React code, as it prevents passing along optional properties without jumping through hoops:

function Foo({ x }: { x?: number }) { ... }

function Bar({ x }: { x?: number }) {
  return <Foo x={x} /> // error
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Question An issue which isn't directly actionable in code
Projects
None yet
Development

No branches or pull requests

7 participants