size-suggestion: collapse `toJSON` methods?

[justingrant]

There are 11 toJSON methods in Temporal. Could ECMA-262 special-case Temporal objects in JSON.stringify to remove the need for all those methods?

[SimenB]

Lots of custom serializers rely on toJSON (e.g. pretty-format, downloaded 80M times a week, https://github.com/jestjs/jest/blob/0d222c1609e616333db16c6cff5de04e04aad222/packages/pretty-format/src/index.ts#L226), not just JSON.stringify.

[littledan]

It'd be unfortunate to add special logic to JSON.stringify; Temporal should follow normal conventions as much as possible. It would be nice if we could reuse the toString method in the toJSON property, but this is risky because of how the key argument is passed to toJSON, and then our toString function takes options. I'd prefer to leave things how they are.

[justingrant]

Meeting 2024-05-23: At the upcoming June 2024 TC39 plenary meeting, we'll propose specifying toJSON to have a single implementation that's shared among all Temporal types, saving 10 methods.

[anba]

The various toJSON methods have different function bodies, so using a single implementations seems strange. valueOf (#2857) has at least the same shared step across all function bodies, but that doesn't apply to toJSON. And if toJSON is changed to a single method, why stop there and have different toString, until, etc. methods?

[ptomato]

My idea was to basically have the body of the shared toJSON be the equivalent of return this.toString(); either with or without a brand check. I believe (but still need to confirm) that toJSON in the current spec text is always equivalent to calling toString with no options.

[ptomato]

I checked this, and the result of toJSON is always equivalent to calling toString with no arguments.

I have an implementation of this here.

It's a bit weird that we would be introducing another observable call, along with all the associated Unlike consolidating the valueOf methods which I think is a no-brainer, I'm neutral about this one. if everyone else thinks the benefits outweigh the drawbacks, and the plenary is OK with it, we should go ahead, but it's maybe a bit more dubious than some of the other things we're proposing.

What I would not want to do is consolidate these methods while eliminating the toString call, because that means we'd basically have a big switch statement based on the type of the receiver. Anba already mentioned that would be strange, and I think that's something implementations can choose to do anyway if they are so inclined, without identity equality between all the toJSON methods.

[anba]

• Date.prototype.toJSON has no brand checks, so for consistency also omit them here?
• Similar to valueOf, I think that if function sharing is needed, it should also apply to other non-Temporal types. So instead of %TemporalToJSON% something like ToJSONFromToString.
• Nit: GetMethod can't return null.

valueOf and toJSON are part of broader object interface, so we really shouldn't create any Temporal-specific rules, but instead if we add new rules (like using a single aliased function for toJSON which delegates to toString), they should also apply to other (future) proposals.

[ljharb]

Date shouldn't be something we need to be consistent with - the brand checks are an improvement and one Temporal needs to keep.

[justingrant]

Would keeping brand checks prevent the function from being re-used by many different ECMAScript built-in objects?

[anba]

Yes, a brand check prevents the function from being reused by possible future proposals. (Unless those proposals extend the brand check with their own types.) In general I don't think we do brand-check followed by generic code, do we? A brand check is always followed by accessing some internal state which is only accessible after a successful brand check.

[justingrant]

Meeting 2024-05-30:

• We need more info about this proposed change's impact on V8. @sffc to talk with @shu about how much savings this would be, and whether there's some other option that can combine implementations without having to change the spec.
• If we do make this change, it's not OK to make observable lookups and calls of this.toString(). It's OK to have a big switch statement by type. (Which could be extended for future proposals.)
• Once we have more info, we can decide whether to make this a late-breaking PR for Helsinki.

[justingrant]

• @sffc to talk with @shu about how much savings this would be, and whether there's some other option that can combine implementations without having to change the spec.

@sffc - any luck on this? Trying to figure out what to say about this one in our Helsinki slides.

[sffc]

The response I received (and @shu can clarify) was that there would likely be a memory savings due to having fewer JSFunctions, but serialization is a hot path and it's questionable whether it's worth deviating from the norm.

I do personally think it is worthwhile discussing / acting plenary about this.

[ptomato]

I think if V8 wants the memory savings, they can get most of it already by multiplexing the toJSON builtins on the C++ side, and in that case we wouldn't have to add a weird anomaly to the spec which would make the multiplexing mandatory for all implementations.

[ptomato]

(In other words, if we're not going to have generic code that looks up toString, I think it's better not to do this. Happy to still present it in the slides, alternatively we could decide to withdraw that commit, whatever...)

[ptomato]

Conclusion from the TC39 plenary of 2024-06-12 is that we will wait for Mozilla to investigate the feasibility/desirability of this, and possibly bring it back at the July plenary.

[dminor]

Our position is that we should make all other proposed changes to reduce the size of Temporal first, and only do this if we get feedback that Temporal is still too large to ship.

[ptomato]

Thanks. Closing this, will reopen if that condition is met.