sveltejs / kit

web development, streamlined
https://svelte.dev/docs/kit
MIT License
18.77k stars 1.96k forks source link

Nested layouts are rendering inside parent layouts for error pages #1576

Closed moisesbites closed 2 years ago

moisesbites commented 3 years ago

Describe the bug

I have a layout.svelte (1) in the root of route directory. I have another layout.svelte (2) more specific in a internal route. So, when I click in a link to a route that not exists yet, the error default error page is renderized in a blank page sometimes, or in the <slot> of __layout.svelte (2) in another times. But, if after that I click in a link to a valid route, the new page is renderized in a new <slot>, as if it were being rendered in the slot (1) , but still maintaining the rendering of slot (2), rendering twice .

Logs

Don't have server logs for that. There is no significant log in console.

To Reproduce

image image image

  "devDependencies": {
    "@sveltejs/adapter-node": "^1.0.0-next.20",
    "@sveltejs/kit": "next",
    "eslint": "^7.27.0",
    "eslint-config-prettier": "^8.3.0",
    "eslint-plugin-svelte3": "^3.2.0",
    "prettier": "~2.3.0",
    "prettier-plugin-svelte": "^2.3.0",
    "svelte": "^3.38.2",
    "svelte-windicss-preprocess": "^4.0.7",
    "windicss": "^3.0.12"
  },

Expected behavior I click in a link. The page not exist. The error page is renderized. I click in a new link, to a page that exists. The page is new renderized.

Information about your SvelteKit Installation:

Diagnostics ``` System: OS: Linux 5.8 Linux Mint 20.1 (Ulyssa) CPU: (4) x64 Intel(R) Core(TM) i5-2400 CPU @ 3.10GHz Memory: 12.14 GB / 23.38 GB Container: Yes Shell: 5.0.17 - /bin/bash Binaries: Node: 16.2.0 - ~/.nvm/versions/node/v16.2.0/bin/node npm: 7.13.0 - ~/.nvm/versions/node/v16.2.0/bin/npm Browsers: Chrome: 91.0.4472.77 Chromium: 90.0.4430.212 Firefox: 88.0.1 npmPackages: @sveltejs/kit: next => 1.0.0-next.107 svelte: ^3.38.2 => 3.38.2 ``` - Your browser `Firefox 88.0.1 (64-bits) (For Linux Mint 20.1)` - Your adapter (e.g. Node, static, Vercel, Begin, etc...) `Node`

Severity How severe an issue is this bug to you? Is this annoying, blocking some users, blocking an upgrade or blocking your usage of SvelteKit entirely? Blocking your usage of SvelteKit entirely. Only if the client refresh the page, the problem is solved.

moisesbites commented 3 years ago

apparently, when an error occurs, the error is running on the entire stack to the root and not stopping at the first __error.svelte on the stack.

bleucitron commented 3 years ago

Nested __error.svelte pages are ignored for 404s, but correctly displayed for actual errors.

bleucitron commented 3 years ago

The current behaviour of rendering root __error page for 404s (instead of nested __error if present) seems to be intended.

If confirmed, docs might need to be updated to mention it.

AsafAgranat commented 3 years ago

@iOiurson could you please elaborate what is behind this intention? It seems counter intuitive considering that the behavior pattern for __layout.svelte files (files of similar genealogy in my opinion) is to render the deepest in stack first.

maxicarlos08 commented 3 years ago

The current behaviour of rendering root __error page for 404s (instead of nested __error if present) seems to be intended.

But why? I am having similar issues and it is quite annoying

bleucitron commented 3 years ago

@AsafAgranat @maxicarlos08 I don't know really.

I guess the intention is that rendering root __error for all 404s is enough, since an unknown url is unknown whatever its position in your tree. You should not need to differentiate between 404s, assumption i don't agree with.

Personally, I also find it counterintuitive.

AsafAgranat commented 3 years ago

@iOiurson I hear you, and we also should remember that the feature is called "error", seemingly handling not only 404s, but a wider spectrum of errors, and errors are almost always context relevant. Nesting error-handling could go a long way in perceptually serving them to users.

Rich-Harris commented 2 years ago

I'm having a hard time understanding this issue without a reproduction. Can you provide one, either in a git repo or on https://node.new/sveltekit?

moisesbites commented 2 years ago

I'm having a hard time understanding this issue without a reproduction. Can you provide one, either in a git repo or on https://node.new/sveltekit?

Hello @Rich-Harris

I tried to reproduce this issue at https://stackblitz.com/edit/sveltekit-8zmejn?file=src/routes/index.svelte, but I think that new version of Svelte at this moment don't have this problem anymore.

In my web application sometimes the error occurs yet under stress tests. But I workaround this issue with window.location.replace().

Thank you all for the effort on this exceptional language/framework.

benmccann commented 2 years ago

It sounds like this issue is longer present, so I will close this issue before long. If you are still having this problem, please provide instructions for reproducing it

cliftonlabrum commented 2 years ago

If I've understood the problem correctly, it's still happening as of SvelteKit 1.0.0-next.304. Here's how to reproduce it.

Create a new skeleton project:

npm init svelte@next my-app
cd my-app
npm install
npm run dev

Create some files and folders for sample routes:

<!-- /routes/__error.svelte --> 
<h1>Root __error.svelte</h1>
<!-- /routes/test/index.svelte --> 
<h1>Test page.</h1>
<!-- /routes/test/__error.svelte --> 
<h1>Test __error.svelte</h1>

Navigate to localhost:3000/test/abc. I would expect to see the contents of /routes/test/__error.svelte for the 404 error, but I see the routes/__error.svelte page. This is not the behavior described in the docs.

bleucitron commented 2 years ago

It would display the /test/__error.svelte if you'd thrown a JS error in /test/index.svelte.

For 404 pages, whatever the path of the unknown page, the root __error.svelte is displayed, as mentioned here: https://github.com/sveltejs/kit/issues/1576#issuecomment-897410507.

This might feel counter-intuitive to some people (at least to me).

If this behaviour is intended, docs might need an update to mention this.

Rich-Harris commented 2 years ago

If I've understood the issue correctly, then SvelteKit is working as intended: https://github.com/sveltejs/kit/issues/2454#issuecomment-1004051345. Have opened #4529

AsafAgranat commented 2 years ago

@Rich-Harris, if the intention of nested __error pages is to get rendered only upon failure to render the corrsponding route, can we test this in any way (trigger a failure)?

mrkishi commented 2 years ago

It seems we got sidetracked a little. @moisesbites couldn't get a reproduction working but described a different error where layouts got duplicated in a single render.

@moisesbites Do you still get that issue on your application? Do you use transitions or any special html manipulation?

moisesbites commented 2 years ago

@mrkishi

It seems we got sidetracked a little. @moisesbites couldn't get a reproduction working but described a different error where layouts got duplicated in a single render.

This is the real error that motivated me to open this issue. image

When a error 404 is rendered, sometimes, a mirror slot appears on the page duplicating the visible layout and components.

@moisesbites Do you still get that issue on your application? Do you use transitions or any special html manipulation?

When a page error occurs, I use a reload page as workaroud for that error. But, I tested https://github.com/sveltejs/kit/issues/1576#issuecomment-1023582190 and I didn't reproduce that error. Transitions or special html, I think not.

In time, if i may make a suggestion, about the 404 error page, I think it's better for the system to render the closest level __error.svelte to a path, because the layout of that path level is, in most cases, relevant to the App.

Rich-Harris commented 2 years ago

If it's not possible to reproduce, then can this issue be closed?

moisesbites commented 2 years ago

If it's not possible to reproduce, then can this issue be closed?

OK. I close. If it happens again, I can open a new issue by referencing this one. Thanks.