-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
chore: have rendered page load only a single script #9681
Conversation
|
if (DEV && target === document.body) { | ||
console.warn( | ||
`Placing %sveltekit.body% directly inside <body> is not recommended, as your app may break for users who have certain browser extensions installed.\n\nConsider wrapping it in an element:\n\n<div style="display: contents">\n %sveltekit.body%\n</div>` | ||
); | ||
} | ||
|
||
// @ts-expect-error | ||
const app = await import('__sveltekit/APP'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we achieve the desired outcome differently? I believe this makes the client.js bundle less cacheable between builds
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it would make it less cacheable. Can you clarify why you think that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory, start
shouldn't change between builds, so it should always be cached after the initial visit, even after a redeploy. app
on the other hand will change frequently (though less frequently with #4482). So if we combine them into a single chunk, the app as a whole gets less cacheable. The current split is very deliberate, for that reason, so we should probably close this PR.
Having said that, I just tried building an app twice, and if kit.version.name
isn't fixed it will result in different start
chunks. I thought we'd fixed that a while back but apparently not. We should investigate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason I used a dynamic import rather than a static import is to make them separate chunks. Ultimately if we want to address the linked issues, we need to have one script that loads all the others.
Though I just realized I need to do this in reverse. The hash on the app chunk will change frequently invalidating the start chunk as I have it now. So I need to swap it and have the app chunk load the more stable start chunk instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to use a dynamic import for that IIUC, you just need to configure Rollup appropriately — if module A statically imports module B, and module B is an entry point, it will work as expected.
Bear in mind that this would create a waterfall for browsers that don't implement modulepreload
. I'm not sure that's a change we want to make. It strikes me that if we want to have a non-code-split mode we'll want a separate bundling strategy specifically for that case
This is a necessary first step for #3882 and by extension #907