Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

request.body is no longer nullish for body-less requests made from load's fetch during SSR #2294

Closed
Conduitry opened this issue Aug 25, 2021 · 4 comments · Fixed by #2295
Closed
Labels
bug Something isn't working

Comments

@Conduitry
Copy link
Member

Conduitry commented Aug 25, 2021

Describe the bug

As of version 1.0.0-next.151, request.body in the handle hook was nullish when the request had no body and was sent via load's fetch during SSR from some page.

As of version 1.0.0-next.152 (#2215), it is now Uint8Array(0) [].

This is inconvenient when you want your handle hook to call fetch to proxy some requests, as you can no longer pass the request object as the second argument to fetch.

Reproduction

Starting from the sample app, update the handle hook as so:

export const handle = async ({ request, resolve }) => {
  console.log(request.body);
  // ...
};

Navigate to the /todos page (which makes API calls), and hit refresh to get a server-rendered page.

Logs

(in the above repro)

Uint8Array(0) []

(in my app)

Request with GET/HEAD method cannot have body
 TypeError: Request with GET/HEAD method cannot have body
     at new Request (file:///foo/node_modules/@sveltejs/kit/dist/install-fetch.js:1254:10)

System Info

System:
    OS: Linux 5.10 Debian GNU/Linux 10 (buster) 10 (buster)
    CPU: (8) x64 Intel(R) Core(TM) i7-10510U CPU @ 1.80GHz
    Memory: 5.92 GB / 12.32 GB
    Container: Yes
    Shell: 5.0.3 - /bin/bash
  Binaries:
    Node: 14.17.5 - /usr/local/bin/node
    Yarn: 1.22.5 - /usr/local/bin/yarn
    npm: 6.14.14 - /usr/local/bin/npm
  npmPackages:
    @sveltejs/adapter-node: next => 1.0.0-next.43 
    @sveltejs/kit: 1.0.0-next.152 => 1.0.0-next.152 
    svelte: 3.42.3 => 3.42.3

Severity

serious, but I can work around it

Additional Information

We've previously blessed calling fetch in handle as a way to deal with API calls in SSR where the app server and the API server live in different places, but are both behind the same reverse proxy when seen from a browser.

@Conduitry Conduitry added the bug Something isn't working label Aug 25, 2021
@Conduitry
Copy link
Member Author

This isn't quite the right reproduction. Closing until I can get this happening outside my project.

@Conduitry
Copy link
Member Author

Reopening. It's specifically calls to handle that result from a server-side call to fetch that have this empty Uint8Array. In these cases, we call handle with a simulated request object rather than initiating a real network request.

@Conduitry Conduitry reopened this Aug 25, 2021
@Conduitry Conduitry changed the title request.body is no longer nullish for body-less requests request.body is no longer nullish for body-less requests made from fetch during SSR Aug 25, 2021
@Conduitry Conduitry changed the title request.body is no longer nullish for body-less requests made from fetch during SSR request.body is no longer nullish for body-less requests made from load's fetch during SSR Aug 25, 2021
@Conduitry
Copy link
Member Author

An only lightly-tested change:

Replace

rawBody: new TextEncoder().encode(/** @type {string} */ (opts.body)),

with

rawBody: opts.body ? new TextEncoder().encode(/** @type {string} */ (opts.body)) : null,

@Conduitry
Copy link
Member Author

opts.body == null ? null : new TextEncoder().encode(/** @type {string} */ (opts.body)) might be better, maybe.

One of the TypeScript nuances here is that opts.body might be null, but rawBody isn't (currently) allowed to be null.

Another question is what to do when someone explicitly passes opt.body as an empty string, or as an empty array. Should those be coerced to null as well, or only do that if they passed undefined/null?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant