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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

chore: have rendered page load only a single script #9681

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions packages/kit/src/exports/vite/dev/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,12 +120,6 @@ export async function dev(vite, vite_config, svelte_config) {
imports: [],
stylesheets: [],
fonts: []
},
app: {
file: `${svelte_config.kit.outDir}/generated/client/app.js`,
imports: [],
stylesheets: [],
fonts: []
}
},
nodes: manifest_data.nodes.map((node, index) => {
Expand Down
11 changes: 4 additions & 7 deletions packages/kit/src/exports/vite/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,7 +331,10 @@ function kit({ svelte_config }) {
name: 'vite-plugin-sveltekit-virtual-modules',

async resolveId(id) {
// treat $env/static/[public|private] as virtual
if (id === '__sveltekit/APP') {
return `${kit.outDir}/generated/client-optimized/app.js`;
}
// virtual modules
if (id.startsWith('$env/') || id.startsWith('__sveltekit/') || id === '$service-worker') {
return `\0${id}`;
}
Expand Down Expand Up @@ -524,7 +527,6 @@ function kit({ svelte_config }) {
});
} else {
input['entry/start'] = `${runtime_directory}/client/start.js`;
input['entry/app'] = `${kit.outDir}/generated/client-optimized/app.js`;

/**
* @param {string | undefined} file
Expand Down Expand Up @@ -731,11 +733,6 @@ function kit({ svelte_config }) {
client_manifest,
posixify(path.relative('.', `${runtime_directory}/client/start.js`)),
false
),
app: find_deps(
client_manifest,
posixify(path.relative('.', `${kit.outDir}/generated/client-optimized/app.js`)),
false
)
};

Expand Down
5 changes: 3 additions & 2 deletions packages/kit/src/runtime/client/start.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,17 +3,18 @@ import { create_client } from './client.js';
import { init } from './singletons.js';

/**
* @param {import('./types').SvelteKitApp} app
* @param {HTMLElement} target
* @param {Parameters<import('./types').Client['_hydrate']>[0]} [hydrate]
*/
export async function start(app, target, hydrate) {
export async function start(target, hydrate) {
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');
Copy link
Member

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

Copy link
Member Author

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?

Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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

const client = create_client(app, target);

init({ client });
Expand Down
14 changes: 6 additions & 8 deletions packages/kit/src/runtime/server/page/render.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,9 +63,9 @@ export async function render_response({

const { client } = manifest._;

const modulepreloads = new Set([...client.start.imports, ...client.app.imports]);
const stylesheets = new Set(client.app.stylesheets);
const fonts = new Set(client.app.fonts);
const modulepreloads = new Set([...client.start.imports]);
const stylesheets = new Set(client.start.stylesheets);
const fonts = new Set(client.start.fonts);

/** @type {Set<string>} */
const link_header_preloads = new Set();
Expand Down Expand Up @@ -329,7 +329,7 @@ export async function render_response({
${properties.join(',\n\t\t\t\t\t\t')}
};`);

const args = [`app`, `${global}.element`];
const args = [`${global}.element`];

if (page_config.ssr) {
const serialized = { form: 'null', error: 'null' };
Expand Down Expand Up @@ -365,10 +365,8 @@ export async function render_response({
args.push(`{\n\t\t\t\t\t\t\t${hydrate.join(',\n\t\t\t\t\t\t\t')}\n\t\t\t\t\t\t}`);
}

blocks.push(`Promise.all([
import(${s(prefixed(client.start.file))}),
import(${s(prefixed(client.app.file))})
]).then(([kit, app]) => {
blocks.push(`import(${s(prefixed(client.start.file))})
.then((kit) => {
kit.start(${args.join(', ')});
});`);

Expand Down
1 change: 0 additions & 1 deletion packages/kit/types/index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1061,7 +1061,6 @@ export interface SSRManifest {
_: {
client: {
start: AssetDependencies;
app: AssetDependencies;
};
nodes: SSRNodeLoader[];
routes: SSRRoute[];
Expand Down
1 change: 0 additions & 1 deletion packages/kit/types/internal.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,6 @@ export interface BuildData {
service_worker: string | null;
client: {
start: AssetDependencies;
app: AssetDependencies;
} | null;
server_manifest: import('vite').Manifest;
}
Expand Down