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

Add strong types for JSON.stringify #21

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions package.json
Expand Up @@ -43,6 +43,11 @@
"import": "./dist/json-parse.mjs",
"default": "./dist/json-parse.js"
},
"./json-stringify": {
"types": "./dist/json-stringify.d.ts",
"import": "./dist/json-stringify.mjs",
"default": "./dist/json-stringify.js"
},
"./fetch": {
"types": "./dist/fetch.d.ts",
"import": "./dist/fetch.mjs",
Expand Down
43 changes: 43 additions & 0 deletions src/entrypoints/json-stringify.d.ts
@@ -0,0 +1,43 @@
interface JSON {
/**
* 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 (Not Used)
* @param space (Not Used)
*/
stringify(
value: undefined | ((...args: any[]) => any),
replacer?: any,
space?: any,
): undefined;
/**
* 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<T = any>(
value: T,
replacer?: (this: any, key: string, value: any) => any,
space?: string | number,
): undefined extends T
? string | undefined
: ((...args: any[]) => any) extends T
? string | undefined
: string;
/**
* 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 an 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<T = any>(
value: T,
replacer?: (number | string)[] | null,
space?: string | number,
): undefined extends T
? string | undefined
: ((...args: any[]) => any) extends T
? string | undefined
: string;
}
74 changes: 74 additions & 0 deletions src/tests/json-stringify.ts
@@ -0,0 +1,74 @@
import { doNotExecute, Equal, Expect } from "./utils";

doNotExecute(() => {
const result = JSON.stringify(undefined);
type tests = [Expect<Equal<typeof result, undefined>>];
});
Comment on lines +3 to +6
Copy link

Choose a reason for hiding this comment

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

functions are also resolved as undefined

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

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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?

Copy link
Author

@sstur sstur Mar 5, 2023

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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

Would greatly appreciate any and all feedback.

Choose a reason for hiding this comment

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

  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.

Choose a reason for hiding this comment

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

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


doNotExecute(() => {
// A function is treated as undefined by the JSON serializer
const fn = () => {};
const result = JSON.stringify(fn);
type tests = [Expect<Equal<typeof result, undefined>>];
});

doNotExecute(() => {
// If the incoming value can possibly be undefined then the return type can
// possibly be undefined
const result = JSON.stringify(5 as undefined | number);
type tests = [Expect<Equal<typeof result, string | undefined>>];
});

doNotExecute(() => {
// If the incoming value can possibly be a function then the return type can
// possibly be undefined
const result = JSON.stringify(5 as number | (() => number));
type tests = [Expect<Equal<typeof result, string | undefined>>];
});

doNotExecute(() => {
// If the type of the incoming value is `any` we can't assume anything about
// it so return the broadest possible type
const result = JSON.stringify(undefined as any);
sstur marked this conversation as resolved.
Show resolved Hide resolved
type tests = [Expect<Equal<typeof result, string | undefined>>];
});

doNotExecute(() => {
// If the type of the incoming value is `unknown` we can't assume anything
// about it so return the broadest possible type
const result = JSON.stringify(undefined as unknown);
type tests = [Expect<Equal<typeof result, string | undefined>>];
});

doNotExecute(() => {
// If the type of the incoming value is "non-nullish" we can be sure it's not
// undefined, but we can't be sure it isn't a function (treated as undefined)
// so return the broadest possible type.
const fn = () => {};
const result = JSON.stringify(fn as Object);
type tests = [Expect<Equal<typeof result, string | undefined>>];
});

doNotExecute(() => {
// If the type of the incoming value contains "object" (meaning non-primitive)
// we can't be sure it isn't a function (treated as undefined) so return the
// broadest possible type.
const value = null as null | string | number | boolean | object;
const result = JSON.stringify(value);
type tests = [Expect<Equal<typeof result, string | undefined>>];
});

doNotExecute(() => {
// If the type of the incoming value contains "Record<K, any>" (in which K is
// any valid index type, e.g. string or symbol) we can't be sure it isn't a
// function so return the broadest possible type.
const value = null as null | string | Record<string, any>;
const result = JSON.stringify(value);
type tests = [Expect<Equal<typeof result, string | undefined>>];
});

doNotExecute(() => {
const value = null as null | string | Record<string, unknown> | any[];
const result = JSON.stringify(value);
type tests = [Expect<Equal<typeof result, string>>];
});