Enforce that endpoint responses can be serialized

[paperclover]

Describe the bug

When using page endpoints, the JSON output of the request isn't serialized before passing it to the component/load(), causing the SSR version of these pages to be different than the client rendered version. To extend this, you can pass any value through the endpoint, including functions and classes (these ones wont pass type checking), and it will be passed as props to the ssr component.

Reproduction

Repo: https://github.com./davecaruso/bug-sveltekit-tojson-endpoints

• Direct link to endpoint
• Direct link to page component

Logs

[no relevant logs]

System Info

System:
    OS: Windows 10 10.0.22000
    CPU: (32) x64 AMD Ryzen 9 5950X 16-Core Processor
    Memory: 41.17 GB / 63.90 GB
  Binaries:
    Node: 17.8.0 - C:\Program Files\nodejs\node.EXE
    Yarn: 1.22.17 - ~\AppData\Roaming\npm\yarn.CMD
    npm: 8.5.5 - C:\Program Files\nodejs\npm.CMD
  Browsers:
    Edge: Spartan (44.22000.120.0), Chromium (101.0.1210.47)
    Internet Explorer: 11.0.22000.120
  npmPackages:
    @sveltejs/adapter-auto: next => 1.0.0-next.42 
    @sveltejs/kit: next => 1.0.0-next.330 
    svelte: ^3.44.0 => 3.48.0

Severity

annoyance

Additional Information

I believe the part of the code that has to be changed is here
https://github.com./sveltejs/kit/blob/master/packages/kit/src/runtime/server/page/load_node.js#L520
as body is just passed through. it should either be passed through JSON.stringify or some helper function to flatten all objects with .toJSON

[Rich-Harris]

Hmm. Since no serialization is happening, I wonder if we should just enforce (albeit only at a type level) that the body returned from a page endpoint handler needs to be a POJO, rather than always traversing the object speculatively.

Is it important that you're able to rely on toJSON happening, or is it more of a shortcut?

[paperclover]

in my use case, i'm using special objects that have a custom .toJSON function (classes representing database objects), so i could just be calling .toJSON() on all of them myself.

Since regular endpoints will automatically handles .toJSON() (there was an issue about it where this was added to the types), I expected page endpoints to work the same.

[paperclover]

opened a potential implementation PR (mainly wanted to learn how to contribute something small), but of course it should be discussed it should be implemented as a feature, or fixed by removing support entirely.

[mrkishi]

I'm slightly inclined to require a POJO as well. It'd be an unfortunate asymmetric quirk, but maybe if we provided your json_value_to_pojo as a built-in helper and added a dev warning pointing users to it whenever they returned a non-POJO it could turn out alright?

[paperclover]

I think that either the function should be automatically used or the user should (dev mode only) get warnings about having toJSON functions. I personally like the automatic side but I understand that it's a small use case that may not be worth adding and slowing down everyone's code.

I also want to remind that one important case that people may run into accidentally is returning Date objects as those have a toJSON method (I think that was why the typedefs were changed allowing normal endpoints to return them).

[mrkishi]

If anything, the Date scenario pushes me further to the POJO-with-dev-warning side, since people not realizing Dates don't survive JSON and getting confused when they get a string would have been reminded they need to return a POJO—which means they'll also need to parse it on the other side.

The difference with standalone endpoints is that you always explicitly treat them as JSON anyway since you're fetching it. Page endpoints, on the other hand, automatically get turned into props, which hides the fact we're travelling through JSON, so the explicitness will end up making things clearer imo.

[paperclover]

that makes sense, you've convinced me it should be the dev warning due to the confusion of automatic type conversion, and how there's warning for returning other invalid structures like a Uint8Array.

I can go an attempt a new PR but I'd like someone else to agree on it before I go put the work into it. Also, I got two question regarding the implementation of a dev-warning:

• We should check the object for more than just toJSON functions, but rather any functions or other non-serializable things, while we're on the topic of searching for toJSON functions, though that isn't as important as typescript will find those.

• Should the json_value_to_pojo be exported from kit? And if so, how would it be imported by the user? SvelteKit doesn't seem to export anything besides types, so maybe we can just leave out the helper? I feel like if you have a Date object that you're using as a prop, you can just .toString/.toJSON right away. However, if it's buried somewhere (perhaps a database fetch returns an object with a Date as one of its properties, something I am running into with Prisma), having a helper to wrap the object in would be simpler than some messy spread syntax.

[k-mack]

FWIW, I recently bumped into this when using Dates in my custom types. I naively assumed the Date property was to going to be deserialized into a Date object. I just ended up re-creating the date on the client (example shown below).

<script lang="ts">
  import type { MyType } from '$models/my_type.type';
  export let obj: MyType;
  obj.date = new Date(obj.date); // typed-ness lost; reconstruct `Date`
</script>

[dummdidumm]

Closed by #5987