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

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

Closed
wants to merge 1 commit into from

Conversation

benmccann
Copy link
Member

This is a necessary first step for #3882 and by extension #907

@changeset-bot
Copy link

changeset-bot bot commented Apr 17, 2023

⚠️ No Changeset found

Latest commit: 3e0a7c2

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

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

@benmccann benmccann changed the title chore: combine app and start chore: have rendered page load only a single script Apr 17, 2023
@benmccann
Copy link
Member Author

Going to close this attempt in favor of a new plan. Probably easier to start over assuming we go that route

Screenshot from 2023-04-17 12-18-57

@benmccann benmccann closed this Apr 17, 2023
@benmccann benmccann deleted the single-entry-point branch April 17, 2023 19:19
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.

3 participants