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

Stuff is not correctly set when a load function returns error #5537

Closed
flavioosh opened this issue Jul 14, 2022 · 4 comments
Closed

Stuff is not correctly set when a load function returns error #5537

flavioosh opened this issue Jul 14, 2022 · 4 comments
Labels
bug Something isn't working help wanted PRs welcomed. The implementation details are unlikely to cause debate load / layout
Milestone

Comments

@flavioosh
Copy link

Describe the bug

When error (or status !== 2xx) is included in the returned object from a load function inside a route, while also providing a stuff object, the $page.stuff store is empty.

Reproduction

https://github.com/flavioosh/sveltekit-stuff-error-repro

Logs

No response

System Info

System:
    OS: Linux 5.18 Arch Linux
    CPU: (16) x64 AMD Ryzen 7 3700X 8-Core Processor
    Memory: 23.23 GB / 31.27 GB
    Container: Yes
    Shell: 5.9 - /usr/bin/zsh
  Binaries:
    Node: 16.16.0 - ~/.nvm/versions/node/v16.16.0/bin/node
    npm: 8.14.0 - ~/.nvm/versions/node/v16.16.0/bin/npm
  Browsers:
    Firefox: 102.0.1
  npmPackages:
    @sveltejs/adapter-auto: next => 1.0.0-next.60 
    @sveltejs/kit: next => 1.0.0-next.374 
    svelte: ^3.44.0 => 3.49.0 
    vite: ^3.0.0 => 3.0.0

Severity

serious, but I can work around it

Additional Information

I'm not sure if this is expected behaviour, but it seems that even if the object returned from a load function contains either error or a non-200 status, we should still get the stuff passed through to the layout or error pages. I'm not sure if I'm doing something wrong but this seems counter-intuitive.

@flavioosh
Copy link
Author

I'm trying to get around this issue by using a store; I use store.set(...) in the load function of the route, and then subscribe to the store with $store in the layout file. A similar issue to the one described above happens, where the store is reverted back to its initial value, but only on the client side. If I disable JavaScript, the store is still populated with the value set in the load function, unlike with stuff. I have created a branch in the above repo called store-alt to show this, not sure if it's a separate issue or not.

@Rich-Harris Rich-Harris added the bug Something isn't working label Jul 19, 2022
@Rich-Harris Rich-Harris added this to the 1.0 milestone Jul 19, 2022
@Rich-Harris Rich-Harris added the help wanted PRs welcomed. The implementation details are unlikely to cause debate label Jul 19, 2022
@flavioosh
Copy link
Author

I've managed to trace a part of this to the normalize function in runtime/load.js, where stuff from the loaded isn't used at all (perhaps intentionally). Using it in the output of the normalize function will then provide it for the error page, and it will be accessible when rendered by the server.

However, during hydration on the client, because only the error page load function is executed, the stuff from the errored out load is never present, and is reset back to {}. From what I can tell, stuff isn't passed to the start (and subsequently _hydrate) function. That's where my ability to traverse the code ends unfortunately.

@dummdidumm
Copy link
Member

dummdidumm commented Jul 26, 2022

There are multiple parts to this:

  1. insize normalize, stuff is not returned
  2. inside the "load correct nodes"-loop of both client and server, stuff is incorrect because it's only the stuff of the last node that loaded without an error
  3. If 1 and 2 are fixed, the hydration on the client still brings up wrong results because the nodes passed from the server are only the layout nodes leading up to the __error page that is selected, so in this case the page is not loaded and therefore its stuff isn't known/used and is missing from the final stuff result.

1 and 2 are rather easy to fix, but 3 is tricky - we would need to replay the logic leading up to the error, so we need all nodes and call their load functions, also those discarded by the error. I don't know if this is a sensible solution. Should this instead be "works as designed"? UI-wise, everything that happened until then is discarded, so we could make the same argument for stuff. If we go that route, we still may have a bug here (haven't tested yet), but it may be that stuff will only contain the stuff from the last layout, missing the merged stuff leading up to it.

@Rich-Harris
Copy link
Member

Closing as outdated due to #5748. +error.svelte files can no longer load things, which means the opportunity for this to break no longer exists

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted PRs welcomed. The implementation details are unlikely to cause debate load / layout
Projects
None yet
Development

No branches or pull requests

3 participants