Fix/Refine Type Inference for Objects with toJSON() Extending Primitives

[nrako]

Ensure objects implementing toJSON(), but also extending primitive types, have their type inferred by toJSON() and not by the extended type. This refinement helps prevent incorrect type inference for specialized objects with enhanced serialization.

Changes:
- Updated type conditions in source/jsonify.d.ts by prioritizing
{toJSON(): infer J} case before primitives.
- Added corresponding test cases in test-d/jsonify.ts.

Example:
This fix resolves issues like the incorrect inference of Jsonify<Temporal.PlainDate>, where Temporal is sourced from @js-temporal/polyfill and where PlainDate is extending Boolean.

import { Temporal } from '@js-temporal/polyfill';
// Incorrectly inferred as Boolean, but string is expected
Jsonify<Temporal.PlainDate>

[nrako]

Hi @sindresorhus, I hope my PR has all the info you need. As used as an example in the commit msg, I ran into the issue when using Temporal.PlainDate from this polyfill: https://github.com/js-temporal/temporal-polyfill.

I think objects that implement {toJSON()} should be prioritized over primitives, and I’m hoping the changes I made just work as they are. Let me know if I missed anything or if there’s anything more to do.

Thanks for checking it out!

[nrako]

@darcyparker – may I bring this PR to your attention as you have been the contributor of #257 which allowed Jsonify to accept non-JSON which implements .toJSON(); perhaps there's an edge case or a logic that I have not anticipated through my fix?

[darcyparker]

@nrako - The meaning of Jsonify (as I intended it) has evolved since I last looked at things closely.

In my view: https://github.com/sindresorhus/type-fest/blob/main/source/jsonifiable.d.ts#L3 is not the right definition. (@itsjohncs FYI). It might be okay... but it's worth re-consideration. I think 2 cases are being clubbed together and there is value in keeping them separate.

There is certainly a use case for a type that is Jsonable and another that is assignable to a JsonValue. Jsonify's original intent was for values that are assignable to JsonValue.

See these cases:
https://github.com/sindresorhus/type-fest/blob/main/test-d/jsonify.ts#L111-L113 The result of toJSON() is assignable. But expectNotAssignable<JsonValue>(nonJsonWithToJSON); is not assignable because its not JSON. Your case of Jsonify<Temporal.PlainDate> is essentially the same thing. It is not a JsonValue. But it's toJSON() result is.

I am also not sure about adding the Map and Set cases added in #410.

The reason Jsonify was created because there isn't a good rule of when to use interface vs type in typescript. JsonValue is defined as a type because of the circular reference. Before typescript supported circular references, there were hacky attempts to define JsonValue using an interface. But using an interface was flawed... People were excited when typescript allowed types to have circular references. But interfaces have benefits too.. For example, tsc type checks faster... You've probably seen cases of typescript: Expression produces a union type that is too complex to represent.. This tends to guide people to use interfaces. But then the problem with an interface for JSON like this:

interface SomethingThatLooksLikeJsonType {
  abc: number;
}
type MyJsonType = {
  nested: SomethingThatLooksLikeJsonType;
}

Neither is assignable to JsonValue or JsonObject. It is so common to have types from some 3rd party package that defines types that are clearly JSON but with an interface... and now you can't use values in a function that expects the input to be assignable to JsonValue. This is what the purpose of Jsonify is for.

type MyJsonType = Jsonify<{
nested: SomethingThatLooksLikeJsonType;
}>

Now you can pass const a: MyJsonType to a function that expects a JsonValue.

I think what you're getting at is that you want something for cases like:

Json.stringify<T extends Jsonable>(value: T): string

where

type Jsonable = { 
  toJSON: <T extends JsonValue>() => T;
}

(I haven't tested Jsonable at all.. but I think it's right.) It is very similar to part of https://github.com/sindresorhus/type-fest/blob/main/source/jsonifiable.d.ts#L3 But the result is JsonValue. And I am not trying to expand the definition of Jsonify.

TLDR; I think people are reading too much into the intent of Jsonify. And that there is a need for Jsonable... and that it is probably better to keep these concept separate and not club them together into Jsonify.

This is open for discussion.. and probably should be spawned into an issue to talk about.

[nrako]

@darcyparker – thanks for taking the time to look into this.

In my view: https://github.com/sindresorhus/type-fest/blob/main/source/jsonifiable.d.ts#L3 is not the right definition.

Did you meant to link source/jsonify.d.ts in the link above?

I think what you're getting at is that you want something for cases like:

Json.stringify<T extends Jsonable>(value: T): string

Actually my usage is more akin to the example of Jsonify, just that in my case I don't use a Date but a Temporal.PlainDate which does implement toJSON but also extend a JSON primitive making the whole type resolution wrong.

import type {Jsonify} from 'type-fest';
import { Temporal } from '@js-temporal/polyfill'


const time = {
    dateValue: Temporal.Now.plainDateTimeISO()
};

// `Jsonify<typeof time>` SHOULD BE the equivalent of `{dateValue: string}` RIGHT NOW we get `{dateValue: Boolean}`
const timeJson = JSON.parse(JSON.stringify(time)) as Jsonify<typeof time>;

To be more specific, I've encountered this issue while trying Remix v2 which is now using Jsonify to infer the serialized data type between the loaders/actions returns (server side payload) and the hooks that consume the payload (both server and client side). See this PR for more infos on when Jsonify has been adopted by Remix: remix-run/remix#6642

I've seen that someone mentioned using Jsonifiable for this before, but quite frankly I'm a bit unclear of how it would play out within Remix's code and AFAICT Jonsify seems justified for the current usage.

[darcyparker]

No - I meant jsonifable. Jsonifable implies jsonify can be used to get a result. But Jsonify was never intended to be used to transform a value to its .toJSON() value. And so I think the current implementation of Jsonifiable is flawed.

Sure Temporal.PlainDate implements .toJSON(), but that doesn't mean it's JSON. It's not. The result of .toJSON() is JSON of course. But the class Temporal.PlainDate is not JSON. JSON.stringify() can receive a Jsonable type like Temporal.PlainDate too... And if you JSON.parse(JSON.stringify()) it's not the same value.

I do understand there is a need for a utility to do a transformation that transforms a value like Temporal.PlainDate to the return value of the .toJSON(). But my opinion is that this shouldn't be Jsonify.

So I think there could be a new type called Jsonable as mentioned above, and that Jsonifable should be re-written and that we need a new type called perhaps a ToJson type to do what you want. (There could be a better name than ToJson utility type.)

Another viewpoint is that we should club the concepts of what I think of as Jsonify and ToJson into one type (fold it into existing Jsonify... but I think that could be a mistake because it would only be safe to use in contexts where you know the .toJSON() method will be used.

[nrako]

Okay, I think my understanding got muddled with all the "Json(able|ify|ifable|Value|...) terms".

I understand that Jonsify has diverged from your initial intent.

I am also not sure about adding the Map and Set cases added in #410.

Nor are you certain about the Map and Set cases.

And that JsonifiableObject does not have the right definition in your opinion:

type-fest/source/jsonifiable.d.ts

Line 3 in 113400b

type JsonifiableObject = {[Key in string]?: Jsonifiable} | {toJSON: () => Jsonifiable};

Or that you believe the Jsonifiable implementation is "flawed".

Thus, you're advocating for a new new Jsonable type:

type Jsonable = { 
  toJSON: <T extends JsonValue>() => T;
}

TLDR; I think people are reading too much into the intent of Jsonify. And that there is a need for Jsonable... and that it is probably better to keep these concept separate and not club them together into Jsonify.

And I caught that you're suggesting spawning a new discussion for this, which I'd certainly agree with, but I don't feel I have the right insights to start this effectively so I can only encourage you to consider doing it.

---

Now, in practical terms for the scope of my changes in this PR, which are meant to fix the issue I encountered; I unfortunately still fail to understand how Jsonable would interplay here, or how it is needed to apply a fix for this problem.

I'm only just catching up with some concepts of type-fest and I'm not trying to question pre-existing implementations, nor introduce new concepts, or significantly change existing ones; I'm just trying to find the simplest and correct path to fix the issue I raised.

@darcyparker – I've asked for your insights because you are the author of #257 which first introduced the handling of .toJSON() in Jsonify and where the decision to tests for extends {toJSON(): infer J} after extends JsonPrimitives has been made.

I would like to emphasize that the essence of this PR only moves the extends {toJSON() case before the extends JsonPrimitives case. Nothing else. This adjustment only aims to rectify the wrong type result that occurs when an object, implementing { toJSON() }, also extends a primitive.

I believe the issue, which this PR aims to fix, would also occur on 4184e18. With my humble understanding, no other changes – questionable or not – made since then have a direct impact on the problem or the solution AFAICT.

[darcyparker]

Sorry - yes this is getting confusing. And I am confusing myself too.

I forgot about #257, and in retrospect I am not sure I like it.

I hear your point about moving the extends {toJSON(): infer J} to after. That's okay with me given toJSON() is in the implementation today. We could consider coming back to the design later.

[nrako]

Okay thanks. It looks we agree on this fix then.

@sindresorhus - how can we move on with this?