Skip to content

feat: statically analyse universal pages and layouts v3 #13684

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

Open
wants to merge 42 commits into
base: main
Choose a base branch
from

Conversation

eltigerchino
Copy link
Member

@eltigerchino eltigerchino commented Apr 7, 2025

closes #12580

Similar to #13669, we use acorn and acorn-ts to analyse the nodes. However, we follow the approach of #13673 where all exports (except load and exports prefixed with _) must have literal values or we fallback to importing the module. Then, we identify which nodes are never used for SSR by computing their page options and any inherited values from their parents. If a node is never used for SSR, that means the server will never access its universal load function (if any) and we can just provide the page option values we retrieved from static analysis. This allows us to avoid importing the module in Node and also avoid importing the component in the build output so that serverless function bundles don't include component code they will never use.


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. Changesets that add features should be minor and those that fix bugs should be patch. Please prefix changeset messages with feat:, fix:, or chore:.

Edits

  • Please ensure that 'Allow edits from maintainers' is checked. PRs without this option may be closed.

Copy link

changeset-bot bot commented Apr 7, 2025

🦋 Changeset detected

Latest commit: 48210cb

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 Minor

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

@svelte-docs-bot
Copy link

@benmccann
Copy link
Member

we follow the approach of #13673 where all exports (except load and exports prefixed with _) must have literal values or we fallback to importing the module

It looks like entries cannot have a literal value, so perhaps it should be excluded as well: https://svelte.dev/docs/kit/page-options#entries. That approach sounds fine to me, but I'm curious as it sounds like there was some other approach considered - can you share what the alternative would be?

@eltigerchino
Copy link
Member Author

eltigerchino commented Apr 15, 2025

we follow the approach of #13673 where all exports (except load and exports prefixed with _) must have literal values or we fallback to importing the module

It looks like entries cannot have a literal value, so perhaps it should be excluded as well: svelte.dev/docs/kit/page-options#entries.

We have to import the module on the server to access entries so we purposely bail out in that case. It's different for load and private exports (prefixed with _) because load is only accessed on the server if ssr is enabled and private exports are never accessed by us.

That approach sounds fine to me, but I'm curious as it sounds like there was some other approach considered - can you share what the alternative would be?

The alternative was #13669 where we're importing modules on demand. This has its own pros and cons such as being more generous before we fallback, logging which export caused the module import on error, but at the same time it introduces more complexities (proxies, requires finding all exports, etc.).

@benmccann
Copy link
Member

more generous before we fallback

What's an example combination of settings that would pass there and not here?

logging which export caused the module import on error

I think as long as we can indicate which file it is that should probably be sufficient. Logging which export it is seems like a nicety, but not necessary to me

@eltigerchino
Copy link
Member Author

eltigerchino commented Apr 21, 2025

more generous before we fallback

What's an example combination of settings that would pass there and not here?

You're right. I checked and there are no combinations that would pass differently. Essentially all page options are being checked by SvelteKit whether or not ssr is false. The only edge case where this would differ is a user exporting something without prefixing it with an underscore _ (which is something we already warn against in the IDE). Having that non-prefixed export with a dynamic value would pass in the other PR but not in this PR (which can be easily changed if we chose to only look for exports matching page option names in this PR).

Therefore, the only difference between that PR and this one is that PR will error later (analysis during build) rather than earlier (rendering the route). In which case, I think I'd prefer it to error earlier (this PR).

logging which export caused the module import on error

I think as long as we can indicate which file it is that should probably be sufficient. Logging which export it is seems like a nicety, but not necessary to me

Good idea. I think I can add that.

EDIT: this already happens as part of the current behaviour.

@eltigerchino eltigerchino marked this pull request as draft April 21, 2025 09:58
@eltigerchino
Copy link
Member Author

eltigerchino commented Apr 21, 2025

Assessing all the page options on dev startup makes the basic app test suite take 3-4 seconds instead of 1.5s. Going to try and make it perform the static analysis lazily similar to when the Vite resolves modules. However, build times will definitely get a couple of seconds longer regardless.

@eltigerchino eltigerchino marked this pull request as ready for review April 23, 2025 03:57
@dummdidumm
Copy link
Member

This is looking promising! Wondering if we should append all errors thrown during loading a universal file with "...if you didn't expect this file to be executed, it was executed because of X. If you don't want it to exexute on the server then make sure that Y" (where X is somehow trying to tell you why it got loaded)

Copy link
Member

@Rich-Harris Rich-Harris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks great — if we delete the failing tests (since we probably don't need to worry about those cases) then I think we can merge this

@benmccann
Copy link
Member

Perhaps we could land #11637 at the same time. I think it's been softened up a bit since the original and improving SPA support at the same time we land it may help soften the blow. I think https://x.com/BenjaminMcCann/status/1844744358429688145 really demonstrates how important it is

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

Successfully merging this pull request may close these issues.

Avoid loading +page.js/+layout.js files on server in SPA mode
4 participants