Correct types for JSON.stringify

[chribjel]

JSON.stringify has some caveats that is not in the standard Typescript implementation.

JSON.stringify(undefined) returns undefined, not string
JSON.stringify(function(){}) returns undefined as well.

This PR fixes that.

[denis-sokolov]

Tightly related: #21, #124.

[mattpocock]

Okay, there seems to be a bit of momentum here. I'm interested in maybe merging this PR, but I have a couple of questions about the implementation.

Do we need a type parameter on JSON.stringify? Could we not use plain function overloads here?

[chribjel]

Okay, there seems to be a bit of momentum here. I'm interested in maybe merging this PR, but I have a couple of questions about the implementation.

Do we need a type parameter on JSON.stringify? Could we not use plain function overloads here?

Yes that is probably a better solution. Type parameters are prone for misuse in this case. will update with function overloads

[cathrinevaage]

(As requested by @nickshanks on twitter, i'm bringing my comment in here)

If we're resetting the JSON.stringify return type, the type should not be based on the value argument alone.
This ignores the replacer argument, which can also impact the return type.

JSON.stringify(undefined, () => ('foo')) // '"foo"'

JSON.stringify('foo', () => (undefined)) // undefined

[WeismannS]

(As requested by @nickshanks on twitter, i'm bringing my comment in here)

If we're resetting the JSON.stringify return type, the type should not be based on the value argument alone. This ignores the replacer argument, which can also impact the return type.

JSON.stringify(undefined, () => ('foo')) // '"foo"'

JSON.stringify('foo', () => (undefined)) // undefined

Interesting, woudl you then say these overloads would be more accurate?

function stringify(val: undefined): undefined;
function stringify(val: (...args: any[]) => any): undefined;
function stringify(val: any, callback: (...args: any[]) => any): string | undefined
function stringify<T>(val: T) : string

[chribjel]

I have now updated with all the necessary overloads to correctly type every* possible case

*i hope

[nickshanks]

I presume the first two overloads could be merged with |.

[mattpocock]

Phenomenal work, and extremely detailed tests.

Could you also add this to the recommended set of rules? Via the recommended.d.ts file.

[mattpocock]

@cathrinevaage Let's move that to a separate issue and discuss separately.

[chribjel]

Phenomenal work, and extremely detailed tests.

Could you also add this to the recommended set of rules? Via the recommended.d.ts file.

Thanks! i believe it already is.

[chribjel]

also i think i already "fixed" what @cathrinevaage mentioned. If it wasnt fixed the previous solution would indeed be broken.

[chribjel]

there is a problem here @mattpocock that i didn't foresee:

if an object or function has a method toJSON we somehow need to know the return type of this method

or we can type the return type to be string | undefined for all input types that MAY have a toJSON method. This would indeed be the most correct solution

[WeismannS]

there is a problem here @mattpocock that i didn't foresee:

if an object or function has a method toJSON we somehow need to know the return type of this method

or we can type the return type to be string | undefined for all input types that MAY have a toJSON method. This would indeed be the most correct solution

I'm pretty sure you meant something like this

type t = "Test"
const s = {
    toJSON : ()  : t => "Test",
    val : "123"
}

function stringify<T>(val: T): T extends { toJSON: (...args: any[]) => infer U } ? U : string {
    if (val && typeof (val as any).toJSON === 'function') {
        return (val as any).toJSON(); 
    }
    return String(val) as any; 
}
const pepe = stringify(s) // type "Test"

This is a pseudocode to what you need,please change as you please, if it isn't, feel free to correct me!

[chribjel]

i have made some changes that i think satisfies all possibilities. Would be nice with a test-suite that check the actual return values honestly. Tried to make sure the problems outlined here have been considered.

if you guys know about some cases where typescript will get this wrong, please let me know.

EDIT: if (terrifyingly) you pass a function as a callback to another function and it adds a toJSON to the function, we have no clue if calling JSON.stringify on that function returns a string or undefined.

Same goes for object.

Therefore we need to always return string | undefined for all functions or objects that can have a toJSON function on it.

[chribjel]

At this point it might just be easier to make the return type string | undefined for everything since most people do it to objects anyways

[WeismannS]

EDIT: if (terrifyingly) you pass a function as a callback to another function and it adds a toJSON to the function, we have no clue if calling JSON.stringify on that function returns a string or undefined.

that is a limitation of JavaScript/TypeScript and not something we should take into account – Ownership isn't a thing here so we shouldn't be handling it
Refer to my previous comment. i believe infering is the right way