type-fest icon indicating copy to clipboard operation
type-fest copied to clipboard

Proposal: Jsonifyable

Open benallfree opened this issue 2 years ago • 19 comments

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.

benallfree avatar Jan 25 '22 14:01 benallfree

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 avatar Jan 25 '22 17:01 sindresorhus

And I think it should be Jsonifyable => Jsonifiable.

sindresorhus avatar Jan 25 '22 17:01 sindresorhus

@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 :)

benallfree avatar Jan 25 '22 17:01 benallfree

JSON.stringify(1n) also fails.

sindresorhus avatar Jan 25 '22 17:01 sindresorhus

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 avatar Jan 25 '22 17:01 sindresorhus

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 https://github.com/sindresorhus/serialize-error/issues/55 which I think could benefit of this type.

papb avatar Jan 26 '22 05:01 papb

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

papb avatar Jan 26 '22 05:01 papb

@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 avatar Jan 26 '22 11:01 benallfree

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? 🤣

benallfree avatar Jan 26 '22 11:01 benallfree

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

sindresorhus avatar Feb 06 '22 04:02 sindresorhus

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

Also can you please answer this?

papb avatar Feb 06 '22 22:02 papb

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

benallfree avatar Feb 06 '22 23:02 benallfree

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

papb avatar Feb 07 '22 01:02 papb

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

benallfree avatar Feb 07 '22 02:02 benallfree

@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 https://github.com/sindresorhus/type-fest/issues/356#issuecomment-1022105267

benallfree avatar Feb 07 '22 02:02 benallfree

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

Yes

Does my suggestion above cover this idea here?

Yes

sindresorhus avatar Feb 15 '22 11:02 sindresorhus

@papb Another use case just came up in https://github.com/remix-run/remix/discussions/2201

benallfree avatar Mar 06 '22 15:03 benallfree

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,
  }

crutch12 avatar Jul 27 '22 10:07 crutch12

image

I had to change all JSONValue to any

tjx666 avatar Sep 21 '22 17:09 tjx666