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

JSON.parse(), JSON.stringify() more specific declarations for #6955 #28270

Conversation

AnyhowStep
Copy link
Contributor

Fixes #6955

I made a few comments in that issue about being unclear about how the whole PR process works,

And it seems I broke 5 tests,

image
image
image

I assume it's because they are expecting the previous signature for JSON.parse() and JSON.stringify() but I've changed them.

I'm not sure how to read the output files in tests/baselines/local and decide if they're okay to baseline-accept (which I assume updates the expected results) but I made a best-effort attempt to look through them and they seemed okay to me.

@msftclas
Copy link

msftclas commented Oct 31, 2018

CLA assistant check
All CLA requirements met.

@AnyhowStep AnyhowStep changed the title Json parse reviver stringify replacer better declaration JSON.parse(), JSON.stringify() more specific declarations for #6955 Nov 1, 2018
@dalen
Copy link

dalen commented Nov 8, 2018

Shouldn't JSON.parse really return an unknown rather than any?

@AnyhowStep
Copy link
Contributor Author

AnyhowStep commented Nov 8, 2018

Ah, yes. That makes sense. Personally, I'm in favour of it now that you've pointed it out but I think it might break code for a lot of people.

[EDIT]

To elaborate, I think changing it to unknown would be good. And breaking existing code in this case isn't actually bad because it would force them to re-examine their code and make type guards and whatnot. Or, if they're lazy, cast with as any.

But I'm not sure how the TS team feels about such a change.

lib/lib.es5.d.ts Outdated
/**
* 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?: (key: string, value: any) => any, space?: string | number): string;
stringify(value: any, replacer?: (this : any, key: string, value: any) => any, space?: string | number): string;
Copy link
Member

Choose a reason for hiding this comment

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

Can you remove the stray space?

Copy link
Member

@RyanCavanaugh RyanCavanaugh left a comment

Choose a reason for hiding this comment

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

OK to merge after Monday

@RyanCavanaugh
Copy link
Member

@typescript-bot test this please

@typescript-bot
Copy link
Collaborator

typescript-bot commented Feb 1, 2019

Heya @RyanCavanaugh, I've started to run the extended test suite on this PR at dd3ed02. You can monitor the build here. It should now contribute to this PR's status checks.

@RyanCavanaugh
Copy link
Member

Nothing relevant in RWC, GTG

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants