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

fix: prevent duplicate <link> module preloads #10442

Merged
merged 6 commits into from Jul 31, 2023
Merged

Conversation

eltigerchino
Copy link
Member

@eltigerchino eltigerchino commented Jul 27, 2023

fixes #10358
fixes #10379

This PR disables Vite's module preload for JS files only (Vite still injects CSS) which adds the preloads on startup in addition to the preloads already added by Kit. I'm not currently aware of any side-effects disabling this may have so this PR might be somewhat naive.

This PR matches the Vite base path setting optimisation with the page rendering base path setting, resulting in identical hrefs. Hence, Vite no longer duplicates asset links that are already in the <head>.

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:.

@changeset-bot
Copy link

changeset-bot bot commented Jul 27, 2023

🦋 Changeset detected

Latest commit: cd55ab3

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 Patch

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

@dummdidumm
Copy link
Member

Crazy, it's failing a seemingly completely unrelated test, but consistently.

@eltigerchino
Copy link
Member Author

eltigerchino commented Jul 27, 2023

Crazy, it's failing a seemingly completely unrelated test, but consistently.

Apparently, disabling Vite's module preload causes client-side navigation to not load the subsequent page's CSS files. Hence, the test is failing because the required CSS rules are not applied.

I've changed it to only disable Vite's JS module preloading, but Kit still relies on Vite to inject the CSS link tag in the head for client-side navigation. Therefore, the duplicate CSS issue is still present and the <link> injected by Vite does not respect our base path setting.

@eltigerchino
Copy link
Member Author

eltigerchino commented Jul 27, 2023

Instead of messing with Vite's module preload option, we might have to prefer absolute asset URLs by default.
@benmccann
This part of page rendering that prefers relative URLs conflicts with this perf optimisation that prefers absolute URLs, causing the asset URLs to be different and Vite unavoidably duplicates them.

@dummdidumm
Copy link
Member

To me it sounds like we should then revert that perf optimisation then for the time being.

@eltigerchino
Copy link
Member Author

I've adjusted so that it is no longer the default, but the optimisation still works if the relative path setting is set to false.

@benmccann
Copy link
Member

the optimisation still works if the relative path setting is set to false

won't that create duplicate module preload <link> elements in that case though?

it seems to me like the better way might be to update that section of code you referred to: https://github.com/sveltejs/kit/blob/926c22d94fe7884dc9bc020ce735a911f50c8509/packages/kit/src/runtime/server/page/render.js#L96C4-L108

@eltigerchino
Copy link
Member Author

eltigerchino commented Jul 29, 2023

the optimisation still works if the relative path setting is set to false

won't that create duplicate module preload <link> elements in that case though?

It shouldn't as long as the base path settings match for both sections of code (either ./ or whatever kit.config.base is).

it seems to me like the better way might be to update that section of code you referred to: 926c22d/packages/kit/src/runtime/server/page/render.js#L96C4-L108

Yes, initially I was inclined to change it so that root-relative was the default, but I'm not sure if that would cause problems for existing apps that relied on the default setting (relative paths)?

The default behaviour as mentioned in the docs:

By default, if paths.assets is not external, SvelteKit will replace %sveltekit.assets% with a relative path and use relative paths to reference build artifacts

We would be changing the default to false.

%sveltekit.assets% and references to build artifacts will always be root-relative paths, unless paths.assets is an external URL

- if (paths.relative !== false && !state.prerendering?.fallback) {
+ if (paths.relative && !state.prerendering?.fallback) {
// only use relative paths if explicitly set in svelte.config.js

Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

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

Agree with that reasoning 👍

@dummdidumm dummdidumm merged commit 0f00498 into master Jul 31, 2023
14 checks passed
@dummdidumm dummdidumm deleted the fix-duplicate-link branch July 31, 2023 11:16
@github-actions github-actions bot mentioned this pull request Jul 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants