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

Proposal: Jsonifyable #356

Closed
benallfree opened this issue Jan 25, 2022 · 19 comments · Fixed by #492
Closed

Proposal: Jsonifyable #356

benallfree opened this issue Jan 25, 2022 · 19 comments · Fixed by #492

Comments

@benallfree
Copy link

I'm hitting a problem where (string | undefined)[] is incompatible with JsonValue.

export type BrokenWrapper<TValue extends JsonValue> = {};

export type MyType = string[];

export type BrokenWrappedType = BrokenWrapper<PartialDeep<MyType>>;

https://codesandbox.io/s/nice-water-lp2n5

Will produce:

Type '(string | undefined)[]' does not satisfy the constraint 'JsonValue'.
  Type '(string | undefined)[]' is not assignable to type 'JsonArray'.
    Type 'string | undefined' is not assignable to type 'JsonValue'.
      Type 'undefined' is not assignable to type 'JsonValue'.  ts(2344)

I think this error is correct because undefined is certainly not allowed in a JSON value.

What I want is a way to express an object that can pass JSON.stringify without error. Ie, Jsonifyable!

It's almost identical, but undefined is allowed as a value:

export type JsonifyableObject = { [Key in string]?: JsonifyableValue };
export type JsonifyableArray = JsonifyableValue[];
export type JsonifyablePrimitive = string | number | boolean | null | undefined;
//                                                                  ^^^ this is the magic
export type JsonifyableValue =
  | JsonifyablePrimitive
  | JsonifyableObject
  | JsonifyableArray;

If you'd like me to turn this into a PR, I'd be happy to. I'm using it in my production code right now.

@sindresorhus
Copy link
Owner

sindresorhus commented Jan 25, 2022

There are a lot more values that are "jsonifiable" though. How do you think we should handle those? For example, Symbol, RegExp, Date, and pretty much any object with a .toJSON() method.

@sindresorhus
Copy link
Owner

And I think it should be Jsonifyable => Jsonifiable.

@benallfree
Copy link
Author

benallfree commented Jan 25, 2022

@sindresorhus Thank you for the feedback. After playing with it a bit more, I think I misunderstood my own use case. Closing.

For anyone who finds their way here, it should be noted that JSON.stringify will only fail if an object has cycles. Unless there is a way in Typescript to detect object cycles, anything is Jsonifiable :)

@sindresorhus
Copy link
Owner

JSON.stringify(1n) also fails.

@sindresorhus
Copy link
Owner

I can see the usefulness of also supporting undefined in the JSON types though. We can keep this open for more feedback. It would need a different name.

@sindresorhus sindresorhus reopened this Jan 25, 2022
@papb
Copy link
Contributor

papb commented Jan 26, 2022

I agree that a Jsonifiable type in the sense of "not throwing" is not really useful (cycles and BigInts only).

But how about a Jsonifiable type in the sense of "not losing information" (in a not-so-strict sense)? This one seems very useful, although I haven't really encountered the need myself (yet?). I think it can be done. We can make a recursive type like JsonValue, but allowing more things, such as Date, Int8Array, while still forbidding things like Error and Set. In this case I think undefined should be allowed because I consider "{ a: 1 }" a perfectly valid representation for { a: 1, b: undefined }, because I think it's an anti-pattern to rely on the "obscure" undefined-but-present quirk. Of course anything with a toJSON function (with appropriate return value) is also allowed.

@sindresorhus I can make a PR.

edit: I've just stumbled upon sindresorhus/serialize-error#55 which I think could benefit of this type.

@papb
Copy link
Contributor

papb commented Jan 26, 2022

I can see the usefulness of also supporting undefined in the JSON types though. We can keep this open for more feedback. It would need a different name.

Does my suggestion above cover this idea here? Or do you think only additionally allowing undefined is also useful in itself? I couldn't think of why. For example, defining a subtype of JsonValue with possibly-missing values already works (playground).

@benallfree
Copy link
Author

benallfree commented Jan 26, 2022

@papb not losing information expresses it perfectly. Or put another way, a type that guarantees the result of JSON.parse is compatible with the type before JSON.stringify.

For example, {foo: ()=>string} would not pass because JSON.parse(JSON.stringify({foo: ()=>'baz'})) is {} which is not compatible with the original type ({foo: ()=>string} and {} are not compatible types). But type {foo?: ()=>string} would pass because {} is compatible with that.

I want to to disallow any values that would lead to an incompatible type after deserialization so I can be sure that what comes out the other end of the wire is type equivalent.

Just for clarity, I think the existing JsonValue type is perfect for describing the result of JSON.parse. But the notion of that result being compatible with the original type is something different :)

@benallfree
Copy link
Author

benallfree commented Jan 26, 2022

Array types like (string|undefined)[] are thorny because JSON.parse(JSON.stringify(['a', undefined, 'b'])) becomes ['a', null, 'b'] on the other side and null is not assignable to undefined. This is where PartialDeep was correctly causing a type failure.

In some sense I think PartialDeep is going a little too far. {foo: string[]} becomes {foo?: (string|undefined)[]} but the user probably just intended {foo?: string[]} which would be fine for serialization. PartialDeepExceptForThoseDarnArrays? 🤣

@sindresorhus
Copy link
Owner

not losing information expresses it perfectly. Or put another way, a type that guarantees the result of JSON.parse is compatible with the type before JSON.stringify.

👍

In some sense I think PartialDeep is going a little too far. {foo: string[]} becomes {foo?: (string|undefined)[]} but the user probably just intended {foo?: string[]} which would be fine for serialization. PartialDeepExceptForThoseDarnArrays? 🤣

I think we could add an optional type parameter option for that: PartialDeep<Foo, {recurseIntoArrays: false}> (with a better option name).

@papb
Copy link
Contributor

papb commented Feb 6, 2022

@sindresorhus So are you OK with me doing a PR as I proposed above?

Also can you please answer this?

@benallfree
Copy link
Author

@papb Consider also whether recurseIntoArrays: false should be the default. I think it probably should be, because it feels unexpected to make array elements nullable rather than just the keys. But now that TS supports tuples I'm not sure which way should be the default. The error it causes is quite subtle, at least for the way I reason through things. It took me a while to discover the root problem.

@papb
Copy link
Contributor

papb commented Feb 7, 2022

@benallfree Ah, yes, I think I agree about recurseIntoArrays: false being default for PartialDeep, but I was talking about Jsonifiable in my last comment, which is what I'm mostly interested in doing a PR for. Although I may do a PR for PartialDeep afterwards maybe.

@benallfree
Copy link
Author

@papb Oh gotcha, yes I’d love to see Jsonifiable, I think that would be an excellent addition.

@benallfree
Copy link
Author

@papb If you do a PR please also consider the case of allowing incompatible types which are coerced to undefined as long as the type allows it. See my comment above #356 (comment)

@sindresorhus
Copy link
Owner

So are you OK with me doing a PR as I proposed above?

Yes

Does my suggestion above cover this idea here?

Yes

@benallfree
Copy link
Author

benallfree commented Mar 6, 2022

@papb Another use case just came up in remix-run/remix#2201

@crutch12
Copy link

I think it would be nice if we had utility type ParsedJson<T>

  type User = {
    name: string,
    parent: User | null,
    sayHello: () => string,
    friends: Map<string, User>,
    birthDate: Date,
  }

  const user = JSON.parse(/* serialized user */) as ParsedJson<User>
  
  // typeof user
  {
    name: string,
    parent: ParsedJson<User> | null,
    sayHello: never,
    friends: {},
    birthDate: string,
  }

@tjx666
Copy link

tjx666 commented Sep 21, 2022

image

I had to change all JSONValue to any

itsjohncs added a commit to itsjohncs/type-fest that referenced this issue Oct 16, 2022
itsjohncs added a commit to itsjohncs/type-fest that referenced this issue Oct 16, 2022
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 a pull request may close this issue.

5 participants