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

Add undefined to JSON.stringify return type #51897

Merged
merged 4 commits into from
Dec 28, 2022
Merged

Conversation

ronyhe
Copy link
Contributor

@ronyhe ronyhe commented Dec 14, 2022

Add undefined to JSON.stringify return type when input value is Function | Symbol | undefined
Fixes #18879

@reverofevil
Copy link

Even though this is kind of better than it was before, there's still a variety of cases when this typing fails.

JSON.stringify({toJSON: () => undefined})

@ronyhe
Copy link
Contributor Author

ronyhe commented Dec 15, 2022

Even though this is kind of better than it was before, there's still a variety of cases when this typing fails.

JSON.stringify({toJSON: () => undefined})

Do you have a suggestion as to how to approach this?

@ronyhe
Copy link
Contributor Author

ronyhe commented Dec 15, 2022

@microsoft-github-policy-service agree company="Wix.com"

@reverofevil
Copy link

reverofevil commented Dec 15, 2022

Do you have a suggestion as to how to approach this?

Something like this.

Once I wrote down "correct" types for JSON methods, and those were utterly dreadful. Correct approach is to reexport them somewhere in your project with types tightened as much as possible (without any undefined, toJSON, replacers and overloads allowed), and avoid spending any time to handle accidental complexity of EcmaScript standard and TS type system. It doesn't pay off.

@ronyhe
Copy link
Contributor Author

ronyhe commented Dec 15, 2022

It doesn't pay off.

Then let's leave the toJSON issue alone. We can add the changes from this PR to improve the current state of affairs without solving everything.
According to the issue people have been tripped up by this

@ronyhe
Copy link
Contributor Author

ronyhe commented Dec 15, 2022

I'm considering extracting out the replacer type:

type JsonReplacer = number[] | string[] | ((this: any, key: string, value: any) => any)

interface JSON {
    /**
     * Converts a JavaScript Object Notation (JSON) string into an object.
     * @param text A valid JSON string.
     * @param reviver A function that transforms the results. This function is called for each member of the object.
     * If a member contains nested objects, the nested objects are transformed before the parent object is.
     */
    parse(text: string, reviver?: (this: any, key: string, value: any) => any): any;
    /**
     * Converts a JavaScript value to a JavaScript Object Notation (JSON) string.
     * @param value A JavaScript value, usually an object or array, to be converted.
     * @param replacer An array of strings and numbers that acts as an approved list for selecting the object properties that will be stringified.
     * @param space Adds indentation, white space, and line break characters to the return-value JSON text to make it easier to read.
     */
    stringify(value: Function | Symbol | undefined, replacer?: JsonReplacer | null, space?: string | number): undefined;
    /**
     * Converts a JavaScript value to a JavaScript Object Notation (JSON) string.
     * @param value A JavaScript value, usually an object or array, to be converted.
     * @param replacer A function that transforms the results.
     * @param space Adds indentation, white space, and line break characters to the return-value JSON text to make it easier to read.
     */
    stringify(value: any, replacer?: JsonReplacer | null, space?: string | number): string;
}

Any thoughts?

@MartinJohns
Copy link
Contributor

@ronyhe What for? It's only used twice, and moving it to a dedicated type is yet another potential break (if someone has such a type defined). Also, the TypeScript team generally doesn't want to add more utility types:

#39522 (comment)

We've opted to not include utility type aliases in the lib unless they're required for declaration emit purposes

@ronyhe
Copy link
Contributor Author

ronyhe commented Dec 15, 2022

@ronyhe What for?

@MartinJohns The original replacer type has two options (not including null). I suppose that's the reason for the existing overload.
In order to make this complete I'll need to add another overload with the second option or use a (less readable, IMO) union type.
So the options here are:

  • Extract to utility type
  • Union type
  • Another overload
    WDYT?

@MartinJohns
Copy link
Contributor

So the options here are:

  • Extract to utility type
  • Union type
  • Another overload

I don't understand this. Extracting it to a utility type it's still a union. And overloads are not interchangeably with unions, they behave very much different.

@ronyhe
Copy link
Contributor Author

ronyhe commented Dec 16, 2022

@MartinJohns Yes, I suppose you're right.
The further I think about, the more I'm coming to the conclusion that the PR should simply remain as-is.
Achieving completeness might prove problematic since the replacer function may actually handle the cases that lead to an undefined result.
I think that the PR in its current state is quite helpful, it will help others avoid a pitfall without adding too much complexity.
As far as I'm concerned this PR is ready for review and for merging.
WDYT?

@sandersn sandersn self-requested a review December 23, 2022 01:06
@sandersn sandersn self-assigned this Dec 23, 2022
@sandersn
Copy link
Member

This looks reasonable to me and I can't think of ways for it to break existing code. I'm going to run some user tests on it to double-check.

@typescript-bot user test this
@typescript-bot test top100
@typescript-bot run dt

@typescript-bot
Copy link
Collaborator

typescript-bot commented Dec 27, 2022

Heya @sandersn, I've started to run the diff-based top-repos suite on this PR at 2da11c0. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Dec 27, 2022

Heya @sandersn, I've started to run the diff-based user code test suite on this PR at 2da11c0. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Dec 27, 2022

Heya @sandersn, I've started to run the parallelized Definitely Typed test suite on this PR at 2da11c0. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

@sandersn Here are the results of running the user test suite comparing main and refs/pull/51897/merge:

Everything looks good!

@typescript-bot
Copy link
Collaborator

@sandersn Here are the results of running the top-repos suite comparing main and refs/pull/51897/merge:

Everything looks good!

@sandersn sandersn merged commit c7f49bc into microsoft:main Dec 28, 2022
@reverofevil
Copy link

That was totally unexpected.

Great job, @ronyhe!

@ronyhe
Copy link
Contributor Author

ronyhe commented Dec 28, 2022

Thanks @polkovnikov-ph and thank you @MartinJohns and @sandersn for the help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Backlog Bug PRs that fix a backlog bug
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Type signature for JSON.stringify does not include undefined in the return type
5 participants