Add strong types for JSON.stringify #21
Conversation
sstur
force-pushed
the
addJsonStringify
branch
2 times, most recently
from
450c945
to
67fff97
Compare
src/tests/json-stringify.ts
Outdated
|
|
||
| doNotExecute(() => { | ||
| const result = JSON.stringify(undefined); | ||
| type tests = [Expect<Equal<typeof result, string | undefined>>]; |
There was a problem hiding this comment.
result here should probably be undefined!
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
I think an extra overload makes sense here.
| doNotExecute(() => { | ||
| const result = JSON.stringify(undefined); | ||
| type tests = [Expect<Equal<typeof result, undefined>>]; | ||
| }); |
There was a problem hiding this comment.
functions are also resolved as undefined
doNotExecute(() => {
const fn = () => {};
const result = JSON.stringify(fn);
type tests = [Expect<Equal<typeof result, undefined>>];
});There was a problem hiding this comment.
Oh, this is a good catch! I didn't even realize that. Checking into it...
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Could you add an overload to catch it being a function? And then only return undefined.
There was a problem hiding this comment.
is it not also all right to assume that a value typed
Record<string, any>is not afunctionat runtime?
It is right to assume that a value typed Record<string, any> could be a function at runtime:
const a: Record<string, any> = () => {}If the return type is specialized to
undefined, then that's not useful at all. The developer might as well delete any calls toJSON.stringifythat return onlyundefined.
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 to
string, then the developer is lulled into a false sense of security. Sure, the result might actually be astring, but what if it isn't? As I described earlier, it is indeed possible for the result to beundefinedbecause 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 | undefinedthan 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.
There was a problem hiding this comment.
is it not also all right to assume that a value typed
Record<string, any>is not afunctionat 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.
-
The type
Record<string, any>usesany, which disables type checking. Change the type toRecord<string, unknown>instead and you'll see the type error.For any type
A, the typeRecord<string, A>should be assignable toRecord<string, unknown>because the typeAis in a covariant position. The type() => voidis not assignable toRecord<string, unknown>. This means that() => voidis not assignable toRecord<string, A>for some typeA, i.e. functions are not records. -
() => voidbeing assignable toRecord<string, any>should be considered a bug. In fact, I went ahead and raised a bug issue for it.() => voidis assignable toRecord<string, any>microsoft/TypeScript#53484Hopefully, 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 toJSON.stringifythat return onlyundefined.This would actually be useful, it will alert the developer to a case when they have accidentally used
stringifyin 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 to
string, then the developer is lulled into a false sense of security. Sure, the result might actually be astring, but what if it isn't? As I described earlier, it is indeed possible for the result to beundefinedbecause 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 | undefinedthan 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
toJSONcase 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.
There was a problem hiding this comment.
@sstur @KotlinIsland Created my own PR for adding type definitions for JSON.stringify, #124.
Would greatly appreciate any and all feedback.
There was a problem hiding this comment.
- The type
Record<string, any>usesany, which disables type checking. Change the type toRecord<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 toRecord<string, unknown>. Thus,Record<string, unknown>could indeed be a function at runtime. All the more reason to not specialize the return type ofJSON.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-eslintsupports 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.stringifywe'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.
There was a problem hiding this comment.
@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.
|
|
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! |
|
@KotlinIsland Added support for return type specialization in my PR, #124. It also correctly handles @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 |
|
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 |
|
@denis-sokolov Shameless plug. Consider using I originally implemented these type definitions in
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 Anyway, I'm not recommending
In conclusion, |
Hmm, I wonder if the very next thing will be an offense 🤔
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. |
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. 🙂
|
The built-in types for
JSON.stringify()are not correct/sound becauseJSON.stringify()will returnundefinedifundefinedis 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!