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

Layout Stack not Fully Reset for src/routes/__layout.svelte #2154

Closed
jsprog opened this issue Aug 10, 2021 · 10 comments · Fixed by #6174
Closed

Layout Stack not Fully Reset for src/routes/__layout.svelte #2154

jsprog opened this issue Aug 10, 2021 · 10 comments · Fixed by #6174
Labels
bug Something isn't working
Milestone

Comments

@jsprog
Copy link

jsprog commented Aug 10, 2021

Describe the bug

According to the official sveltekit documentation, adding __layout.reset.svelte inside a nested folder should reset layout stack and prevent inheriting from upper level layouts. Unfortunately, the browser is receiving some logs from the module context of __src/routes/__layout.svelte. ssr logs are clean and not affected.

Reproduction

Logs

No response

System Info

System:
    OS: Linux 4.19 LMDE 4 (debbie) 4 (debbie)
    CPU: (4) x64 Intel(R) Core(TM) i5-3570K CPU @ 3.40GHz
    Memory: 10.01 GB / 15.56 GB
    Container: Yes
    Shell: 5.0.3 - /bin/bash
  Binaries:
    Node: 14.17.1 -
    npm: 6.14.13 -
  Browsers:
    Chrome: 92.0.4515.131
    Firefox: 68.8.0esr

Severity

annoyance

Additional Information

  • ssr logs are not affected by this.
  • other nested layout files are not affected by this, and their children __layout.reset.svelte are fully resetting them.
@anudeepreddy
Copy link

anudeepreddy commented Aug 10, 2021

Hey,
I am trying to understand this issue. In the manifect.js file, is there any particular reason to export the fallback as array of components? I think that is causing the issue, not really sure as I am a complete beginner. And also if someone could explain me when is fallback used and how it will be of much help.

Thanks ✌

const c = [
	() => import("..\\..\\..\\src\\routes\\__layout.svelte"),
	() => import("..\\components\\error.svelte"),
	() => import("..\\..\\..\\src\\routes\\index.svelte"),
	() => import("..\\..\\..\\src\\routes\\login\\__layout.reset.svelte"),
	() => import("..\\..\\..\\src\\routes\\login\\index.svelte")
];

const d = decodeURIComponent;

export const routes = [
	// src/routes/index.svelte
	[/^\/$/, [c[0], c[2]], [c[1]]],

	// src/routes/login/index.svelte
	[/^\/login\/?$/, [c[3], c[4]], []]
];

export const fallback = [c[0](), c[1]()];

@rmunn
Copy link
Contributor

rmunn commented Aug 10, 2021

The Svelte docs for context="module" mention that that script block will be run when the module is first evaluated. In other words, code in that script block turns into code at the top level of the Javascript module that it's compiled into. Which means that that code will run when the module is imported.

And @anudeepreddy, you're right that the fallback setup in the manifest.js file is causing the top-level layout to be imported. The fallback in that manifest is the fallback error component, which is mentioned in https://kit.svelte.dev/docs#layouts-error-pages in this paragraph:

SvelteKit provides a default error page in case you don't supply src/routes/__error.svelte, but it's recommended that you bring your own.

Since the fallback error page uses the root layout as its rendering, that means that the root layout is always imported, and any module-level side effects (such as a console.log call) in root layout will always be run during the process of rendering the first page you load.

However, any subsequent layout pages that are reset will not be imported (and their module context will not be run). For example, I took @jsprog's repro code and added the following files to it:

src/routes/foo/__layout.svelte
src/routes/foo/bar/__layout.reset.svelte
src/routes/foo/bar/index.svelte

The contents of those layout pages were similar to the other layout pages in the repro: a console.log call in both the module and instance scripts. And when I loaded http://localhost:3000/foo/bar/ and looked in the console log, I saw that the foo/__layout module had not been loaded; not even its module-context script had been executed.

So I believe this is not actually a bug in the layout reset process, but rather a side effect of the error components always using the root layout, which means that the root layout is always imported and so its module context script is always executed.

(Note that if you provide your own src/routes/__error.svelte as opposed to using the default fallback one that Svelte-Kit provides, that export const fallback = [...] line is still going to be in the manifest, because the error component will always use the root layout no matter what.)

@rmunn
Copy link
Contributor

rmunn commented Aug 10, 2021

That comment turned out a bit long. The short version is that layouts are being properly reset (see my foo/bar/__layout.reset.svelte example), but the root layout is always loaded (at least, it's always imported, which causes module-context code to run) so that any page-load errors can be rendered with src/routes/__error.svelte.

Therefore, if you have any side-effecting code in the module-context script of your root layout, it will always run because that module will always be imported. But at any level below the root, the layout module is not imported unless it's going to be used for rendering, so module-context script at any level below the root won't be run if there's a lower __layout.reset file.

@anudeepreddy
Copy link

anudeepreddy commented Aug 10, 2021

Shouldn't we export the fallback array as functions, and change the usage in Renderer so that we can import them when they are actually required and not on every page reload as we do now. This approach also seems to solve #2130.

@jsprog
Copy link
Author

jsprog commented Aug 10, 2021

The current implementation plays against the purpose behind the existence of code splitting. Developers should be allowed to bring some complex components to the main layout without bloating the login page (e.g: complex header containing menus and dialogs).

The following screenshots were taken after updating the reproduction code with more files:

1- main

2- login

@jsprog
Copy link
Author

jsprog commented Aug 10, 2021

Another Note: both $lib/module.js and $lib/Component.svelte in the updated reproduction code are imported by the instance script of src/routes/__layout.svelte, and other layout reset files are not resetting these imports.

@rmunn
Copy link
Contributor

rmunn commented Aug 11, 2021

According to #1356 (comment) it's deliberate to have the fallback components (the root layout and error components) imported during initial load, even if they're not used, precisely because they are fallback components. If something goes wrong in the process of loading a page, one of the likely reasons why it went wrong is a network failure, in which case loading the error components will fail too if they aren't already in the user's browser. That is why the root layout and error components are imported as soon as possible, so that they will be in memory in the user's browser and available to use without any further network traffic. Which means that if the network goes down five seconds after the user loads your site's first page, there's at least something that Svelte-Kit can display to inform them that the site isn't working right.

Therefore, this behavior is unlikely to change, and the only real fix to this issue is going to be to add a note to the documentation saying "Don't put side effects in the root layout module script, as they might be run when you don't expect them to run."

@johndunderhill
Copy link

This is a problem for me. While I can avoid side effects by not placing any actions at the module level in __layout.svelte, this shouldn't be imported if it isn't used because:

  • Someone else might put code there, and it's not technically wrong to do so.
  • It breaks the semantics JavaScript modules.
  • It's horribly confusing.

No page should be hardwired to the default layout, which is highly likely to be used for non-trivial purposes. The same applies to __error.svelte (see 4582).

In the current case, it's a bit trickier because the fallback page is automatically generated, but I think it would be better not to use any layout by default, and provide a way to configure the layout for the error page by:

  • Creating a magical named layout such as layout-fallback.svelte;
  • Not using the default fallback page if __error.svelte is provided and 4582 is implemented (allows one to choose the layout for the error page, as in __error@root.svelte); or
  • Making an option in the configuration file to specify the layout for the default fallback page.

As of now, I'm seriously considering changing all my pages to use explicitly named layouts to avoid any more problems with __layout.svelte being imported and/or used when it shouldn't be. This has been a nightmare. Unexpected behavior is causing a lot of problems for people and likely will continue to be an issue for new users.

@Rich-Harris Rich-Harris added this to the 1.0 milestone Apr 26, 2022
@benmccann
Copy link
Member

@jsprog are you still facing this since the introduction of named layouts? If so, could you update the description and reproduction?

@dummdidumm dummdidumm added bug Something isn't working and removed awaiting submitter labels Jul 22, 2022
@dummdidumm
Copy link
Member

dummdidumm commented Jul 22, 2022

Confirmed that this still happens in the latest version. The culprit is this:

// we import the root layout/error components eagerly, so that
// connectivity errors after initialisation don't nuke the app
const default_layout = components[0]();
const default_error = components[1]();

I'm not sure what the best fix here is without sacrificing this behavior - only do it when we know there's only a single root layout? Is this logic even necessary? If there are connectivity issues, what's gained from rendering the root layout? It makes more sense for the root error for me, but when named errors are implemented, we'll get the same problem there. The only case where this is benefitial is when you navigate the app, connection breaks down, and you haven't loaded the default root layout yet.

Is it possible to somehow prefetch the code, but not execute it yet?

dummdidumm added a commit that referenced this issue Aug 24, 2022
#6124

Closes #6196 (using (groups) and/or composition)
Closes #5763 (root layout guaranteed to always exist + await parent())
Closes #5311 (+page@.svelte)
Closes #4940 (no longer possible to get into this situation)
Closes #2154 (only a single root layout now)

Co-authored-by: Simon Holthausen <simon.holthausen@vercel.com>
Co-authored-by: Dominik G. <dominik.goepel@gmx.de>
Co-authored-by: Ignatius Bagus <ignatius.mbs@gmail.com>
Co-authored-by: Conduitry <git@chor.date>
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
7 participants