Jsonify support for optional object keys, unserializable object values

[frontsideair]

Related discussion: https://github.com/remix-run/remix/pull/3276#discussion_r923949087

Summary: Currently we remove object entries that have unserializable values. This mimics JSON.stringify behavior for simple types, but when object value is a sum type we end up with a never, which is an unintended consequence.

I'd like to address this issue, but there's room for discussion. Take these for example:

type T1 = {
  prop?: string
}

type T2 = {
  prop: string | undefined
}

I think we should preserve the optionality in the first case. (We actually do that so there's no need for change here.) In the second case, there are a few options but the one that I like most is to make it optional too (and remove the undefined).

My rationale is this: If the prop value is string, we end up with a string. If the prop value is undefined (or any other unserializable value), JSON.stringify omits this entry. This resembles the optional, since if the entry is omitted it won't be available when the object's properties are enumerated.

The problem is this is tricky to achieve. We need to keep the optional keys optional, but make required keys optional if the value has an unserializable variant, and unserializable types should be removed from the sum type in value. (If the value does not have a serializable variant, the whole entry should be omitted; this is the current behavior.) TypeScript mapped types don't allow this out-of-the-box, you have to operate on all keys at once. But a quick search brought me to this SO answer, which shows that it's possible and not very hacky.

Before I embark on this quest, I wanted to frame the problem and discuss my thought process. Remix already depends on type-fest and there's interest in replacing the homemade solution with Jsonify when remaining edge cases are fixed. (I intend to do it as well.)

Please let me know if you have any feedback or objections.

[sindresorhus]

the one that I like most is to make it optional too (and remove the undefined).

👍

[tshddx]

there are a few options but the one that I like most is to make it optional too (and remove the undefined).

I agree as well.

The only other viable option I can think of would be to prevent loader from returning any object type with a required field that might be undefined. Those should be easy enough to detect (if I correctly understand what I'm doing in this TS playground) But that seems to go against the current approach, which is to allow loader to return things like Date that aren't properly JSON-serializable, and transform the return types of useLoaderData into what they will actually be after the trip through stringify and parse.

But even if that were easy to implement and prevented runtime errors, I think it would be inconvenient. I believe people will fairly frequently encounter object types with properties that are required but which may be undefined. It seems to be very common when consuming GraphQL APIs with generated TS types.

[frontsideair]

I'm glad we have an agreement, I'll start the work soon.

Regarding the other option @tshddx, I'd like to leave people to use whatever they want like you mentioned. Also I'd like Jsonify to be as feature complete as possible. Thanks for mentioning it though!

[pcattori]

In Remix, we already make properties that are union including undefined into optionals (see remix-run/remix#3766 and remix-run/remix#3879 ), so for what it's worth we think its a good way to go too! 😄

[frontsideair]

I didn't follow up with the updates in Remix, looks great! I didn't have a lot of bandwidth lately, I still intend to do it but not in the near future unfortunately. If someone else wants to take it over though, I can try to provide support and guidance.

My process would be to copy some tests from Remix repo and make them pass.