Update JSON.stringify#29744
Conversation
To ban the the input value 'undefined' passing in undefined returns undefined. Therefore making the return type incorrect. We could also add an overload for a type that takes undefined and returns undefined but that's redundant anyway. NOTE: functions aren't allowed to be inputs either, but I don't know how to not allow functions but allow all other functions in Typescript.
|
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'. |
|
Looked at some of the failing tests and they are legitimate problems that need to be fixed. JSON.stringify should not be called with undefined. I'll need help fixing those issues, or I can just close this PR and someone else can pick this up. |
| * @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?: (this: any, key: string, value: any) => any, space?: string | number): string; | ||
| stringify(value: Object | string | number | boolean | symbol | null, replacer?: (this: any, key: string, value: any) => any, space?: string | number): string; |
There was a problem hiding this comment.
nit: JSON.stringify(Symbol.for('foo')) === undefined, so symbol should be dropped
There was a problem hiding this comment.
problem is return type from stringify not allowed value types. I think it should be undefined | string instead. This can be addressed by adding specific overloads. Please correct me if I am wrong.
There was a problem hiding this comment.
string | undefined with --strictNullChecks would force you make an existential check at runtime, even when it's statically provable what the return type would be
There was a problem hiding this comment.
but not when you have overloads.
stringify(value: Object | string | number | boolean | null, replacer?: (this: any, key: string, value: any) => any, space?: string | number): string;
stringify(value: undefined | Function | symbol, replacer?: (this: any, key: string, value: any) => any, space?: string | number): undefined;There was a problem hiding this comment.
i misunderstood; based on the type signature you wrote i thought you were advocating for a single api. my apologies!
you are correct, an overloaded api would be strictly better. i checked your definitions against MDN and they look correct too.
thank you :)
There was a problem hiding this comment.
Shouldn't it be object instead of Object?
And it would useful to move this into a JSONStringifyable type so it can be reused.
There was a problem hiding this comment.
you're correct; object narrows out symbol where Object does not:
declare class Foo {
foo(o: object): boolean;
foo(o: symbol | Function): string;
}
new Foo().foo(Symbol.for('')); // string
declare class Bar {
bar(o: Object): boolean;
bar(o: symbol | Function): string;
}
new Bar().bar(Symbol.for('')); // booleanunfortunately, i found a different issue:
new Foo().foo(() => { }); // boolean, should be string
new Bar().bar(() => { }); // boolean, should be stringthis would be most readily resolved by moving the overload with Function above the "happy path" overload. unsure if that's the right decision or if there's an alternative i'm missing.
| */ | ||
| stringify(value: any, replacer?: (number | string)[] | null, space?: string | number): string; | ||
| stringify(value: Object | string | number | boolean | symbol | null, replacer?: (number | string)[] | null, space?: string | number): string; | ||
| } |
|
What issue is tracking this change? |
|
@RyanCavanaugh #18879, though i'm pretty sure OP came from this tweet |
|
Haven't forgotten, I'll make the change to remove symbol from the list and try and fix the failing tests. |
|
Closing due to long-term CI failure See also comments in #29962 (comment) if anyone wants to open a fresh PR |
|
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
6 participants
To ban the the input value 'undefined' passing in undefined returns undefined. Therefore making the return type incorrect.
We could also add an overload for a type that takes undefined and returns undefined but that's redundant anyway.
NOTE: functions aren't allowed to be inputs either, but I don't know how to not allow functions but allow all other functions in Typescript.
Fixes #