Skip to content

Update JSON.stringify #29744

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.

Sign up for GitHub

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

Closed
wants to merge 4 commits into from
Closed

Update JSON.stringify #29744

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4069f01
Update JSON.stringify
nmn Feb 5, 2019
23f8bc0
Update JSON.stringify
nmn Feb 5, 2019
9f9aff1
Merge branch 'patch-2' of github.com:nmn/TypeScript into patch-2
nmn Feb 5, 2019
1bd9772
Revert change to lib/
nmn Feb 5, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/lib/es5.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1043,14 +1043,14 @@ interface JSON {
* @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?: (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;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: JSON.stringify(Symbol.for('foo')) === undefined, so symbol should be dropped

Copy link

@chyzwar chyzwar Feb 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't it be object instead of Object?

And it would useful to move this into a JSONStringifyable type so it can be reused.

Copy link

@zackschuster zackschuster Feb 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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('')); // boolean

unfortunately, i found a different issue:

new Foo().foo(() => { }); // boolean, should be string
new Bar().bar(() => { }); // boolean, should be string

this 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.

/**
* 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 a 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: 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;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see above


/**
Expand Down