-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
Fix return type for JSON.stringify(any) -> string #38574
Conversation
It looks like you've sent a pull request to update our 'lib' files. These files aren't meant to be edited by hand, as they consist of last-known good states of the compiler and are generated from 'src'. Unless this is necessary, consider closing the pull request and sending a separate PR to update 'src'. |
@typescript-bot test this |
Heya @RyanCavanaugh, I've started to run the parallelized community code test suite on this PR at f26c011. You can monitor the build here. |
Heya @RyanCavanaugh, I've started to run the extended test suite on this PR at f26c011. You can monitor the build here. |
The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master. |
True, perhaps there’s a compromise to be made. I’ll have a go at removing the `any` input overload, or at least make that also return `any`, so that it is at least not a falsehood.
|
@RyanCavanaugh Can we see how the update does with user tests? I'm thinking that |
Hopefully this can be merged soon, because this issue is bugging me a lot. |
Same here. I've shot myself in the foot because of |
Is this not progressing due to conflicts/an issue with the fix, or has there just not been time to merge it yet? I.e, is the blocker with the author or with maintainers? Trying to evaluate whether to submit a PR myself or not as was just bitten by this bug. (Not trying to spam here – just wondering if I can assist) |
@jupiter Are you sure these types are correct? I think it doesn't handle at least
Update. Found that |
stringify(value: any, replacer?: (number | string)[] | null, space?: string | number): string; | ||
stringify( | ||
value: any, | ||
replacer: (this: any, key: string, value: any) => number | string | boolean | null | object, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JSON.stringify({}, () => () => {})
uses this overload and returns string
instead of undefined
.
): string; | ||
stringify( | ||
value: any, | ||
replacer?: (this: any, key: string, value: any) => any, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think replacer
shouldn't be optional here.
stringify( | ||
value: undefined | Symbol, | ||
replacer?: (number | string)[] | null, | ||
space?: any |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why space
is any
here?
Funny enough, even the best types I could imagine are not enough. Due to some
|
Can this, or some other PR that fixes #18879 , get looked at again? |
Unfortunately, we never finished reviewing this PR. It is pretty old now, so I'm going to close it to reduce the number of open PRs. |
Fixes #18879
Problem
Current type definitions falsely assert that
JSON.stringify(...)
always returns a string.JSON.stringify(...)
actually returnsundefined
whenever the input value isundefined
, a Function, or a Symbol (see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/stringify). It also returnsundefined
when a provided replacer function returns any of these types for the root value.The fix
We provide overloads to ensure that when valid JSON values are provided, the return type is
string
. When an invalid value is provided, the return type isundefined
as per runtime behaviour. When the input value type isany
or when a replacer function is passed the return type isstring | undefined
.