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

[fix] render error page without root layout if that failed to load #6155

Closed
wants to merge 1 commit into from

Conversation

dummdidumm
Copy link
Member

Fixes #4815
Fixes #3068

Please don't delete this checklist! 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 pnpm changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

@changeset-bot
Copy link

changeset-bot bot commented Aug 22, 2022

🦋 Changeset detected

Latest commit: 987d94f

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

@Rich-Harris
Copy link
Member

I definitely don't think we should introduce this behaviour — it's effectively non-deterministic.

With the new layouts proposal, it would be straightforward to have the fallback error page render inside a blank layout (or a layout that is guaranteed not to error, because it doesn't have an accompanying load).

That leaves the question of what to do if the root layout does error. Provided we've given app developers the tools to avoid that scenario (which #6124 does), then a plain 500 response seems perfectly acceptable during SSR. If it happens on the client, the path of least resistance might be to simply reload the page.

@dummdidumm
Copy link
Member Author

How is this non-determenistic? If the root layout fails, it's not used - this is equivalent to a blank layout with the difference that the user doesn't have to create a group just to guarantee that his root layout never errors. If you talk about non-determinism on the client due to the try-catch - that could be adjusted so that the same logic as on the server applies: if we detect that the root layout errored, not use it, else use it.

@Rich-Harris
Copy link
Member

It's non-deterministic because failures are non-deterministic. As an app author I shouldn't have to design my error page such that it will work both inside and outside the layout depending on factors outside my control

@dummdidumm
Copy link
Member Author

But how likely is it that you designed your error page that way? It could only fail if it references $page.data in a way thinking the data from the layout is present. I argue this is less likely than being surprised by "why do I get a blank error message" followed by "ugh, I need to create an empty layout and therefore a needless group just for that?"

@Rich-Harris
Copy link
Member

It's very unlikely, and that's the problem!

@dummdidumm
Copy link
Member Author

It's unlikely that the root layout errors. It's even more unlikely that someone uses things from the root layout in their root error page. It's very likely that someone wants to show something better than blank text if the root layout fails. Therefore everyone who wants that is forced to use a (group) (under the new proposal) to get around that. I don't think it's good that you are forced to create a blank layout and a otherwise unnecessary (group) just to be safe when rendering the error page - this feels like "losing" one layout to safety.

@Rich-Harris
Copy link
Member

Totally agree that there should be something better than blank text, but what that thing is absolutely has to be predictable. There will always be cases where we need to bail completely (even if layout load succeeds, the error page itself could reference {data.nonexistent.property}; if it fails, then even data.existent.property would blow up), and so if a plain text response is unacceptable then that's the problem we need to tackle.

One possible idea: an optional src/error.html that can contain things like %sveltekit.error.message%. If unspecified, falls back to a default error page that's slightly nicer than the one we have now. Unlike +error.svelte, we can make a cast-iron guarantee that this will render successfully.

@dummdidumm
Copy link
Member Author

People have expressed the desire previously to have a error page break out of the complete layout stack (including the root layout). Can we satisfy both needs by introducing a special +error@<something>.svelte which means "this error page is rendered without any layout"? error.html would be just that but you couldn't reuse your Svelte components there.

@Rich-Harris
Copy link
Member

I think it's better to use the existing primitives. My reaction to 'but then you'll need to put everything inside a (main) group' is 'so what?' In practice it doesn't make it any harder to navigation around the codebase — it feels superfluous in the case where you're only using the blank layout for error pages, but it avoids all these problems...

  • what if I want to share some CSS between my error page and the rest of the app? it's easier to do that in a root layout than to duplicate it in my error page
  • what if I want the analytics code in my root layout afterNavigate function to keep working as the user navigates to/from/between errors?
  • what if I later discover another use for my blank layout?

...and that's before we try to answer the design question (what is the <something>?), let alone the documentation and maintenance burden that comes from creating new scenario-solving primitives.

error.html is solving a different problem, and I think it's worth considering on its own merits (but probably in a separate issue).

@Rich-Harris
Copy link
Member

closing in favour of #6367

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants