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

load functions runs on both server and client #8302

Closed
chrisseto opened this issue Dec 31, 2022 · 0 comments · Fixed by #8547
Closed

load functions runs on both server and client #8302

chrisseto opened this issue Dec 31, 2022 · 0 comments · Fixed by #8547
Labels
documentation Improvements or additions to documentation

Comments

@chrisseto
Copy link

Describe the bug

Despite the what is stated by the documentation svelte kit apps will re-fetch data via the load function unless specific methods of the response object are accessed.

Reproduction

This can be easily reproduced by implementing your own JSON unmarshaling, not that you would want to do that. I stumbled across this bug because I was using a GRPC Web Client which decodes the payload from body directly.

// Place in any +page.ts/js file

export const load = (async ({ fetch }) => {
    const resp = await fetch('/some-endpoint');

  // You wouldn't ever want to do this. What's important is that we're not accessing .text or .json
  const chunks: Array<any> = [];
  for await (let chunk of resp.body) {
      chunks.push(chunk)
  }

  return JSON.parse(Buffer.concat(chunks).toString('utf-8'));
}) satisfies PageLoad;

Logs

N/A

System Info

System:
    OS: macOS 13.0.1
    CPU: (8) arm64 Apple M1
    Memory: 349.42 MB / 16.00 GB
    Shell: 3.5.1 - /opt/homebrew/bin/fish
  Binaries:
    Node: 18.11.0 - /opt/homebrew/bin/node
    Yarn: 1.22.17 - /opt/homebrew/bin/yarn
    npm: 8.19.2 - /opt/homebrew/bin/npm
  Browsers:
    Brave Browser: 107.1.45.133
    Safari: 16.1
  npmPackages:
    @sveltejs/adapter-auto: next => 1.0.0-next.90
    @sveltejs/adapter-node: ^1.0.0 => 1.0.0
    @sveltejs/adapter-static: ^1.0.0-next.48 => 1.0.0-next.48
    @sveltejs/kit: next => 1.0.0-next.571
    svelte: ^3.53.1 => 3.53.1
    vite: ^3.2.4 => 3.2.4

Severity

serious, but I can work around it

Additional Information

What's been most frustrating about tracking this down is the phrasing of the documentation. It heavily implies, if not outright states, that the serialized data is what is returned from load. However, load will always be called on both the client and the server. Svelte's fetch function is what memoizes the requests but only under certain circumstances. If my understanding is correct, I'm happy to update the documentation myself.

Additionally, it would be nice to add in some detection to svelte's fetch proxy to log a warning to users if a request is made but can't be memoized so they don't have to go down this rabbit hole themselves.

I'm currently working around the issue by using a modified GrpcWebFetchTransport that accepts a fetch override and I've further customized the unary method to call .text and then feed the result as a ReadableStream to readGrpcWebResponseBody.

				const text = await fetchResponse.text()
				const stream = new ReadableStream({
					start(ctrl) {
						// Re-encode our text as Bytes as
						// readGrpcWebResponseBody expects bytes.
						const encoder = new TextEncoder()
						ctrl.enqueue(encoder.encode(text));
						ctrl.close();
					}
				})
@dummdidumm dummdidumm added the documentation Improvements or additions to documentation label Dec 31, 2022
chrisseto added a commit to chrisseto/kit that referenced this issue Jan 2, 2023
    Previously, calling the sveltekit supplied fetch method would return
    a proxy around fetch that overrode certain methods. Accessing these
    methods would push the response into the fetched array which would
    then be used on the client side to avoid re-fetching data. This
    method required overriding every method on fetch that could read the
    body and could leave developers stumped if they happened to read
    .body from a different method. This commit opts to attach a
    transformer to the responses original body and pushes into fetched
    once the body has been completely read. This ensures that all
    methods of consuming .body will be appropriately recorded.

    Notably, this strategy requires encoding the body as a base64'd array of
    bytes rather than raw text which introduces a minor overhead for
    serialization and deserialization.

    Fixes sveltejs#8302
chrisseto added a commit to chrisseto/kit that referenced this issue Jan 2, 2023
    Previously, calling the sveltekit supplied fetch method would return
    a proxy around fetch that overrode certain methods. Accessing these
    methods would push the response into the fetched array which would
    then be used on the client side to avoid re-fetching data. This
    method required overriding every method on fetch that could read the
    body and could leave developers stumped if they happened to read
    .body from a different method. This commit opts to attach a
    transformer to the responses original body and pushes into fetched
    once the body has been completely read. This ensures that all
    methods of consuming .body will be appropriately recorded.

    Notably, this strategy requires encoding the body as a base64'd array of
    bytes rather than raw text which introduces a minor overhead for
    serialization and deserialization.

    Fixes sveltejs#8302
dummdidumm added a commit that referenced this issue Feb 8, 2023
Rich-Harris added a commit that referenced this issue Feb 28, 2023
* docs: state management

- docs about state management
- more helpful error message when trying to run a SvelteKit store on the server

closes #8524

* fix

* fix the fix

* update

* fix link

* Update documentation/docs/20-core-concepts/60-state-management.md

Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>

* Update packages/kit/src/runtime/app/stores.js

Co-authored-by: Tomasz Olędzki <tomecko@users.noreply.github.com>

* move details to form actions, add url section

* shorten, do not use form in example

* remove now obsolete paragraph, add info on when params and url can change

* links

* changeset, link fix

* info on when which load function runs

* fix

* closes #8302

* Update documentation/docs/20-core-concepts/50-state-management.md

Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>

* details, example

* Update documentation/docs/20-core-concepts/20-load.md

Co-authored-by: Harry Allen <66224939+HarryAllen1@users.noreply.github.com>

* Update documentation/docs/20-core-concepts/20-load.md

* Update documentation/docs/20-core-concepts/50-state-management.md

* docs: state management part 2 (#9239)

* updates

* fix broken link

* fix file annotations

* tweak

* Update documentation/docs/20-core-concepts/50-state-management.md

* Update documentation/docs/20-core-concepts/50-state-management.md

Co-authored-by: Rich Harris <richard.a.harris@gmail.com>

* Update documentation/docs/20-core-concepts/50-state-management.md

---------

Co-authored-by: Simon H <5968653+dummdidumm@users.noreply.github.com>

---------

Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
Co-authored-by: Tomasz Olędzki <tomecko@users.noreply.github.com>
Co-authored-by: Harry Allen <66224939+HarryAllen1@users.noreply.github.com>
Co-authored-by: Rich Harris <hello@rich-harris.dev>
Co-authored-by: Rich Harris <richard.a.harris@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants