Add strong types for JSON.stringify

[sstur]

The built-in types for JSON.stringify() are not correct/sound because JSON.stringify() will return undefined if undefined is passed in (or function or symbol). Many devs don't know this and TS types don't reflect this possibility, resulting in a false sense of null safety and often runtime exceptions in prod.

This fixes that using conditional types. If there's a better way to express this other than conditional types please let me know and I'll update this PR to reflect it.

Thanks!

[mattpocock]

result here should probably be undefined!

[sstur]

You're right. I hesitated to use a nested conditional type here for this edge case, opting for the simpler typedef (that is still sound), but technically yes we can tighten this up to be undefined if we are 100% sure the arg passed in is undefined (and not just extends undefined since any would fit that bill).

[mattpocock]

I think an extra overload makes sense here.

[sstur]

Done.

[zaverden]

functions are also resolved as undefined

doNotExecute(() => {
  const fn = () => {};
  const result = JSON.stringify(fn);
  type tests = [Expect<Equal<typeof result, undefined>>];
});

[sstur]

Oh, this is a good catch! I didn't even realize that. Checking into it...

[sstur]

Hmm, so it turns out this strange behavior of JSON.stringify() treating a function as undefined sorta throws a wrench in the works, because a function can be assigned to Record<string, any>, meaning it's hard to be certain that we have an object that is not a function.

I've update this PR to account for that possibility but be aware this makes it so that many more situations will fall back to string | undefined vs just string.

I think we should consider ignoring the "function treated as undefined" edge case and give up some soundness for developer ergonomics. It would still be far more correct than the in-built types, but would not be perfectly sound (choosing pragmatism over soundness is the long standing posture of TS anyway).

Thoughts @mattpocock?

[sstur]

To put this another way:
Many legit values (that are never intended to be a function) in the real world will likely have the type Record<string, any> and if we go for soundest possible types for JSON.stringify() then passing in those real-world values will result in string | undefined being returned, likely confusing the average TS dev who doesn't realize why. Is this a tradeoff we want to make?

[mattpocock]

Could you add an overload to catch it being a function? And then only return undefined.

[KotlinIsland]

is it not also all right to assume that a value typed Record<string, any> is not a function at runtime?

It is right to assume that a value typed Record<string, any> could be a function at runtime:

const a: Record<string, any> = () => {}

playground

If the return type is specialized to undefined, then that's not useful at all. The developer might as well delete any calls to JSON.stringify that return only undefined.

This would actually be useful, it will alert the developer to a case when they have accidentally used stringify in an invalid case.

If the return type is specialized tostring, then the developer is lulled into a false sense of security. Sure, the result might actually be a string, but what if it isn't? As I described earlier, it is indeed possible for the result to be undefined because TypeScript is unsound.

That argument could be applied to literally any value:

function foo(x: string) {
    // we say x is a string, but someone passed null to this function!
    console.log(x.toLowerCase());
}
const a: string | null = null
foo(a!)

As you can see, it's easier to make the return type always be string | undefined than it is to create specialized type definitions for all the possible edge cases.

I don't think it's a far-fetched idea to specialize this function, even the toJSON case could be accounted for in the conditional return type.

[aaditmshah]

is it not also all right to assume that a value typed Record<string, any> is not a function at runtime?

It is right to assume that a value typed Record<string, any> could be a function at runtime:

const a: Record<string, any> = () => {}

There are two problems with this argument.

1. The type Record<string, any> uses any, which disables type checking. Change the type to Record<string, unknown> instead and you'll see the type error.

   For any type A, the type Record<string, A> should be assignable to Record<string, unknown> because the type A is in a covariant position. The type () => void is not assignable to Record<string, unknown>. This means that () => void is not assignable to Record<string, A> for some type A, i.e. functions are not records.

2. () => void being assignable to Record<string, any> should be considered a bug. In fact, I went ahead and raised a bug issue for it.

   () => void is assignable to Record<string, any> microsoft/TypeScript#53484

   Hopefully, we'll get some clarity on why this issue is occurring. If we get an official answer that this is indeed the expected behavior then I'll be happy to concede this point to you.

That being said, I can see how Object.assign(() => {}, someRecord) would have the type (() => void) & Record<string, unknown> which is indeed assignable to Record<string, unknown>. Thus, Record<string, unknown> could indeed be a function at runtime. All the more reason to not specialize the return type of JSON.stringify.

If the return type is specialized to undefined, then that's not useful at all. The developer might as well delete any calls to JSON.stringify that return only undefined.

This would actually be useful, it will alert the developer to a case when they have accidentally used stringify in an invalid case.

You can use a linter to catch that error. typescript-eslint supports type-aware linting rules. Even better, a linter will inform you that you've made a mistake. In an IDE, it will show you squiggly lines indicating a warning or an error.

Anyway, this is not a strong argument for having specialized return types. It's not worth the added complexity, especially when you have another tool that can solve this problem for you better.

If the return type is specialized tostring, then the developer is lulled into a false sense of security. Sure, the result might actually be a string, but what if it isn't? As I described earlier, it is indeed possible for the result to be undefined because TypeScript is unsound.

That argument could be applied to literally any value:

function foo(x: string) {
    // we say x is a string, but someone passed null to this function!
    console.log(x.toLowerCase());
}
const a: string | null = null
foo(a!)

Except, in the case of JSON.stringify we're only writing the type definition and not the implementation of the function. So, we have to be extra careful when writing the type definition because it won't be verified by the compiler. Thus, it's easy to have edge cases which are incorrect. Going back to my earlier example, stringifying any object with toJSON could potentially return undefined.

const foo = JSON.stringify({ toJSON: () => {} });
const bar = JSON.stringify({ toJSON: () => () => {} });
const baz = JSON.stringify({ toJSON: () => Symbol() });

Writing the type definitions for handling all the edge cases will be very complex. So, even if you do get it correct, which is a very big if, you still need to think whether it's worth all the effort. IMHO, it's not worth the effort.

Just keep it simple and return string | undefined. You get added safety, and it's not much of an inconvenience for developers who are confident that the return type is only string. If you don't want to use the non-null assertion operator because it's unsafe, you could always just throw an error when you get undefined.

const maybeString = JSON.stringify(someValue); // string | undefined

if (typeof maybeString === "undefined") {
  throw new TypeError("JSON.stringify failed to produce a string");
}

// The type of `maybeString` has been narrowed to `string`.

As you can see, it's easier to make the return type always be string | undefined than it is to create specialized type definitions for all the possible edge cases.

I don't think it's a far-fetched idea to specialize this function, even the toJSON case could be accounted for in the conditional return type.

Please, be my guest. I would love to see the type definitions required for handling all the edge cases.

[aaditmshah]

@sstur @KotlinIsland Created my own PR for adding type definitions for JSON.stringify, #124.

Would greatly appreciate any and all feedback.

[DetachHead]

1. The type Record<string, any> uses any, which disables type checking. Change the type to Record<string, unknown> instead and you'll see the type error.

the example he was responding to used Record<string, any>

That being said, I can see how Object.assign(() => {}, someRecord) would have the type (() => void) & Record<string, unknown> which is indeed assignable to Record<string, unknown>. Thus, Record<string, unknown> could indeed be a function at runtime. All the more reason to not specialize the return type of JSON.stringify.

just because it shouldn't be specialized in that case doesn't mean it shouldn't be specialized in other cases. for example primitives such as string and number can be specialized to always return string because we know there's no way they could be functions

You can use a linter to catch that error. typescript-eslint supports type-aware linting rules. Even better, a linter will inform you that you've made a mistake. In an IDE, it will show you squiggly lines indicating a warning or an error.

i don't get why you'd use a linter for an error that the compiler can detect. imo we should aim to catch as many errors at compiletime as possible, to reduce the need for additional tools like eslint

in the case of JSON.stringify we're only writing the type definition and not the implementation of the function. So, we have to be extra careful when writing the type definition because it won't be verified by the compiler.

i agree, but that doesn't mean we shouldn't do it at all. as long as we have tests and verify that each edge case is covered, i see no reason not to do it

Writing the type definitions for handling all the edge cases will be very complex. So, even if you do get it correct, which is a very big if, you still need to think whether it's worth all the effort. IMHO, it's not worth the effort.

Please, be my guest. I would love to see the type definitions required for handling all the edge cases.

i don't think it's that complicated at all. here's my attempt:

type JSONablePrimitive = number | string | boolean;

type JSONStringifyResult<T> =
  | string
  | (
      T extends JSONablePrimitive
        ? never
        : undefined extends T
          ? undefined
          : T extends { toJSON: () => JSONablePrimitive }
            ? never
            : T extends object
              ? undefined
              : never
    );

sure it looks gross, but if you're used to conditional types it's just four if statements and a union (and i'm sure it could be rewritten in a way that looks nicer. in fact it looks like you already did in #124). perhaps i missed something but i can't imagine it getting much more complicated than this.

[aaditmshah]

@DetachHead I agree. I changed my mind and solved it too, 2 days ago. Created a separate PR for it. See, #124. My PR improves both JSON.parse and JSON.stringify. It even provides strong types for the reviver in JSON.parse and the replacer function in JSON.stringify. Furthermore, it correctly handles values with the toJSON method. Finally, it passes all the tests in this PR. I would highly recommend that you check it out. Would love any and all feedback.

[KotlinIsland]

• Typescripts issue for this is here Type signature for JSON.stringify does not include undefined in the return type microsoft/TypeScript#18879

[sstur]

Thanks @KotlinIsland! As you probably discovered, a correct typedef for JSON.stringify was attempted and even landed in TS core, but it was rolled back because it broke too much real-world code. According to RyanCavanaugh in his comment here this is not likely to make it into core.

I suppose stuff like this is a big part of why ts-reset exists!

[aaditmshah]

@KotlinIsland Added support for return type specialization in my PR, #124. It also correctly handles toJSON, constructor functions, and symbols.

@sstur I also copied your test cases into my PR and all of them pass. In addition, I added several more test cases for the replacer function. If you're happy with the changes that I made, we could close this PR in favor of mine.

[denis-sokolov]

Allow me to add a vote of support for this. I have recently experienced a bug in our codebase because of the incorrectly lax type of JSON.stringify.

For what it’s worth, I recommend against always returning string | undefined. The amount of non-null assertion operators it has added in our codebase when we tried it was too large, and led to basically completely ignoring this strict type. It’s okay to have a few false negatives, but not all of them.

[aaditmshah]

@denis-sokolov Shameless plug. Consider using better-typescript-lib instead of ts-reset. Version 2.3.0 of better-typescript-lib added strong type definitions for JSON.parse, JSON.stringify, and fetch().json(). It also added better type definitions for promises. I can vouch for these type definitions because I implemented and tested them myself.

I originally implemented these type definitions in ts-reset.

1. ✨ Use JsonValue instead of unknown #121
2. 💥 Add strong typing for JSON.parse when reviver is specified #123
3. ✨ Add strong typing for JSON.stringify #124

However, my PRs were summarily dismissed by @mattpocock. Matt's a brilliant guy, and he's doing god's work by spreading the gospel of sound typing in TypeScript. But, he's not open to new ideas, which is why I'm doubtful of his open-source leadership skills.

On the other hand, the author of better-typescript-lib, @uhyo, is a genuinely nice guy who listens to others people's ideas. And, the second biggest contributor to better-typescript-lib, @graphemecluster, is a veritable type wizard, just like Matt. I've also seen him answer questions on https://es.discourse.group/. He's a smart guy.

Anyway, I'm not recommending better-typescript-lib to you just because of it's contributors. I'm recommending it to you because I honestly believe that it's superior to ts-reset in every metric.

1. It doesn't require users to define a reset.d.ts ambient type declaration file. If you're using npm or yarn, then all you need to do is install better-typescript-lib as a dev dependency, and you're golden. If you're using pnpm, as I do, then you will also need to configure pnpm to hoist @typescript/* packages in order to use better-typescript-lib.
2. ts-reset doesn't actually replace the built-in type definitions. It just adds additional type overloads to functions. Hence, it's still possible to shoot yourself in the foot with ts-reset. On the other hand, better-typescript-lib actually replaces all the built-in type definitions with entirely new type definitions. It's able to do this because of an obscure new feature introduced by TypeScript 4.5. This means that you will only see the new and improved type definitions in your IDE when you use better-typescript-lib. If you use ts-reset, you will still see the old type definitions.

In conclusion, better-typescript-lib is objectively more well-designed than ts-reset. In addition, it provides strong type definitions for JSON.parse, JSON.stringify, and fetch().json() today. And last but not least, it's maintained by an excellent open-source leader, uhyo, who listens to his users and responds to issues and pull requests in a timely manner. Hence, you can expect to receive good support going forward.

[sstur]

No offense to him. He's a brilliant guy [...]

Hmm, I wonder if the very next thing will be an offense 🤔

he's not open to new ideas, which is why I'm doubtful of his open-source leadership skills. [Some other guy] is a genuinely nice guy who listens

I guess I should have seen that coming; whenever someone opens with "No offense" it's likely it will be followed by something offensive 🤣

Anyway, better-typescript-lib looks interesting, will check it out.

[aaditmshah]

No offense to him. He's a brilliant guy [...]

Hmm, I wonder if the very next thing will be an offense 🤔

he's not open to new ideas, which is why I'm doubtful of his open-source leadership skills. [Some other guy] is a genuinely nice guy who listens

I guess I should have seen that coming; whenever someone opens with "No offense" it's likely it will be followed by something offensive 🤣

Thank you for the writing tip. Fixed. If you don't want to offend people, then never write "no offense". I'll make that a habit. 🙂