[chore] allow EndpointOutput response objects with toJSON property

[JeanJPNM]

Before submitting the PR, please make sure you do the following

• It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
• This message body should clearly illustrate what problems it solves.
• Ideally, include a test that fails without this PR but passes with it.

Tests

• Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

• If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpx changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

This pull request allows users to return objects that have a toJSON method (its return value defines the json that represents the object)

[changeset-bot]

🦋 Changeset detected

Latest commit: d414f03

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@sveltejs/kit	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[benmccann]

What's the motivation behind this change? How would you use it?

[JeanJPNM]

When stringifying an object javascript will check if it has a toJSON method and will stringify the returned value of the function instead of the object.

I saw it was necessary because even though Date objects can be stringified, they aren't compatible with the current type signature of JSONValue.

And this also masks the problem with interface bodies in my case because mongodb documents have a toJSON method that returns the document data as json

[benmccann]

Okay, I see now that JSONValue is used only for the output body. There's no standard way to parse a Date from JSON, but at the same time you can call toJSON on a lot

It's weird that DefaultBody doesn't accept a string. I wonder if the better fix might be to accept a string and have the user call .toJSON themselves when it's not a standard JSON object. That could make them more aware of how they're serializing the response

[JeanJPNM]

I didn't say that string isn't compatible with DefaultBody, I meant that Date isn't compatible with DefaultBody, and yes tecnically you can call a Date constructor and pass as an argument the string representing the date.

Also, JSON.strinfify does call toJSON automatically (if the object has such method)

[benmccann]

My first reaction is that it's a feature, not a bug, that Date isn't compatible with DefaultBody because it can't easily be read back on the other end with JSON.parse. I don't think that the default way a Date is turned into a string is a very efficient encoding and would probably want to be explicit abut converting it to millis or something. As long as you can manually convert it and return it, I think that should probably be sufficient

[benmccann]

Although, then again, if you've explicitly defined a toJSON method on your object we should probably respect that

[JeanJPNM]

That's a good point, being explicit about the shape of the body can make the results more predictable, but manually calling toJSON can also get annoying sometimes.

In my opinion, if someone does put a custom object inside the response body, they would be aware of how it's parsed (if they need to at all) on the client

[JeanJPNM]

Also, I think there should be separate types for json input and output, since they have different constraints

[ignatiusmb]

As you've said, it might be better to separate these types, as the input value has no use of having toJSON method in it. We can separate just the method itself and extends Output with it, keeping JSONValue right now as is.

[ignatiusmb]

I think just extending the Output type argument of RequestHandler should be enough, instead of creating a duplicate separate with only { toJSON } as the differentiator

[JeanJPNM]

I don't think its possible since simply extending output has the following behavior:

const get: RequestHandler = () => {
  return {
    body: new Date() // works
  }
}
const post: RequestHandler = () => {
  return {
    body: {
      date: new Date() // fails
    }
  }
}

[JeanJPNM]

JSONBody makes it possible because it is recursive, and applies to all the nested values inside the response body. And the value returned by a toJSON method needs to be JSONValue

[ignatiusmb]

Oops, totally overlooked that. In any case, we should do something about the duplication.

Looking at it again, I think the separate input and output doesn't really apply here as JSONValue is only used as the output, so I see no reason to create another type. Am I missing something here?

[JeanJPNM]

Both are necessary, I didn't find a way to remove the function signature from JSONResponse recursively, so JSONValue is still needed. The only thing we can do to remove some of the duplication would be a JSONPrimitive type, like this:

type JSONPrimitive = string | boolean | number | null

[ignatiusmb]

We can separate just the function itself into its own ToJSON type and exclude it from the final type (repl)

[JeanJPNM]

This has an edge case:

The following code is valid, even though a toJSON method shouldn't be allowed to be on a value returned by another toJSON method.

const foo: JSONValue = {
	toJSON(){
		return {
			toJSON(){
				return 12;
			}
		}
	}
}

repl

[ignatiusmb]

Let's reuse the output as the value then (repl)

[JeanJPNM]

Ok, that works better than my solution

[benmccann]

there's enough edge cases here that I wonder if we should finally setup our first test for the type definitions to test this

[JeanJPNM]

How do we do it tho?

[benmccann]

this is one project I remember seeing before that tested its types: https://github.com/chartjs/chartjs-plugin-datalabels/tree/master/types/test

[JeanJPNM]

Where do you want to put the tests?

[benmccann]

Putting them in the types directory probably makes sense. E.g. maybe for internal.d.ts we call it internal.spec.js or something

[benmccann]

Ah, I may have given you some bad advice about putting the tests in the /types directory. @ignatiusmb points out to me that then they'll get published in the package which we may not want. He also said there'd been some past discussion about using https://github.com/SamVerschueren/tsd for TypeScript tests.

So I don't know what you want to do given that. If you want to just drop the last few commits we could merge this without the tests and then maybe take a stab at doing testing along those lines in a new PR?

[ignatiusmb]

Thanks for the reverts and comments, we don't really put comments on types other than those in helper. It's nice to also have them in other types, but it would make more sense if it's a public facing type for the comments, like what we have for ambient modules. For simple ones used internally, a meaningful word should be sufficient.