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: revert mjs extension usage by default, make it an option #9179

Merged
merged 12 commits into from
Feb 25, 2023
5 changes: 5 additions & 0 deletions .changeset/grumpy-apricots-admire.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@sveltejs/kit': patch
---

fix: include .mjs files in precompression
5 changes: 5 additions & 0 deletions .changeset/light-oranges-complain.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@sveltejs/kit': patch
---

fix: revert mjs extension usage by default, make it an option
2 changes: 1 addition & 1 deletion packages/kit/src/core/adapt/builder.js
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ export function create_builder({
return;
}

const files = await glob('**/*.{html,js,json,css,svg,xml,wasm}', {
const files = await glob('**/*.{html,js,mjs,json,css,svg,xml,wasm}', {
cwd: directory,
dot: true,
absolute: true,
Expand Down
1 change: 1 addition & 0 deletions packages/kit/src/core/config/index.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ const get_defaults = (prefix = '') => ({
},
inlineStyleThreshold: 0,
moduleExtensions: ['.js', '.ts'],
output: { preloadStrategy: 'modulepreload' },
outDir: join(prefix, '.svelte-kit'),
serviceWorker: {
register: true
Expand Down
4 changes: 4 additions & 0 deletions packages/kit/src/core/config/options.js
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,10 @@ const options = object(

outDir: string('.svelte-kit'),

output: object({
preloadStrategy: list(['modulepreload', 'preload-js', 'preload-mjs'], 'modulepreload')
}),

paths: object({
base: validate('', (input, keypath) => {
assert_string(input, keypath);
Expand Down
1 change: 1 addition & 0 deletions packages/kit/src/core/sync/write_server.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ export const options = {
embedded: ${config.kit.embedded},
env_public_prefix: '${config.kit.env.publicPrefix}',
hooks: null, // added lazily, via \`get_hooks\`
preload_strategy: ${s(config.kit.output.preloadStrategy)},
root,
service_worker: ${has_service_worker},
templates: {
Expand Down
11 changes: 5 additions & 6 deletions packages/kit/src/exports/vite/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -507,6 +507,9 @@ export function set_building() {
}
}

// see the kit.output.preloadStrategy option for details on why we have multiple options here
const ext = kit.output.preloadStrategy === 'preload-mjs' ? 'mjs' : 'js';

new_config = {
base: ssr ? assets_base(kit) : './',
build: {
Expand All @@ -516,12 +519,8 @@ export function set_building() {
input,
output: {
format: 'esm',
// we use .mjs for client-side modules, because this signals to Chrome (when it
// reads the <link rel="preload">) that it should parse the file as a module
// rather than as a script, preventing a double parse. Ideally we'd just use
// modulepreload, but Safari prevents that
entryFileNames: ssr ? '[name].js' : `${prefix}/[name].[hash].mjs`,
chunkFileNames: ssr ? 'chunks/[name].js' : `${prefix}/chunks/[name].[hash].mjs`,
entryFileNames: ssr ? '[name].js' : `${prefix}/[name].[hash].${ext}`,
chunkFileNames: ssr ? 'chunks/[name].js' : `${prefix}/chunks/[name].[hash].${ext}`,
assetFileNames: `${prefix}/assets/[name].[hash][extname]`,
hoistTransitiveImports: false
},
Expand Down
11 changes: 6 additions & 5 deletions packages/kit/src/runtime/server/page/render.js
Original file line number Diff line number Diff line change
Expand Up @@ -273,12 +273,13 @@ export async function render_response({
);

for (const path of included_modulepreloads) {
// we use modulepreload with the Link header for Chrome, along with
// <link rel="preload"> for Safari. This results in the fastest loading in
// the most used browsers, with no double-loading. Note that we need to use
// .mjs extensions for `preload` to behave like `modulepreload` in Chrome
// see the kit.output.preloadStrategy option for details on why we have multiple options here
link_header_preloads.add(`<${encodeURI(path)}>; rel="modulepreload"; nopush`);
head += `\n\t\t<link rel="preload" as="script" crossorigin="anonymous" href="${path}">`;
if (options.preload_strategy !== 'modulepreload') {
Copy link
Member

Choose a reason for hiding this comment

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

hmm, is the rel=modulepreload link always added ? shouldn't it be either one or the other?

Copy link
Member Author

@dummdidumm dummdidumm Feb 24, 2023

Choose a reason for hiding this comment

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

The modulepreload equivalent is the header links above. Since it's a header it doesn't hurt I think (and it always was like this), but you're right we could put it inside an if block

Copy link
Member

Choose a reason for hiding this comment

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

Chrome doesn't respect preload headers, only the <link>. This might be a bug in Chrome, but I haven't got round to investigating and/or filing a ticket. Either way, I think we need to keep the modulepreload link headers for Chrome's benefit

Copy link
Member Author

Choose a reason for hiding this comment

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

Wait, really? This means that chrome didn't get preload behavior previously, too, because that logic I implemented is basically the one we had prior to your PR which introduced the link preload. Mhmmm.
Isn't there also a related issue about the link headers being too large and crashing something? Do we need another "linkheaders" option?

Copy link
Member

Choose a reason for hiding this comment

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

Not sure I follow? we previously added modulepreload links to the HTTP header and still do (and should)

Copy link
Member

Choose a reason for hiding this comment

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

as for the 'link headers being too large' thing, that's why we introduced the preload option to resolve

head += `\n\t\t<link rel="preload" as="script" crossorigin="anonymous" href="${path}">`;
} else if (state.prerendering) {
head += `\n\t\t<link rel="modulepreload" href="${path}">`;
}
}

const blocks = [];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -502,7 +502,7 @@ test.describe('Prefetching', () => {
} else {
// the preload helper causes an additional request to be made in Firefox,
// so we use toBeGreaterThan rather than toBe
expect(requests.filter((req) => req.endsWith('.mjs')).length).toBeGreaterThan(0);
expect(requests.filter((req) => req.endsWith('.js')).length).toBeGreaterThan(0);
}

expect(requests.includes(`${baseURL}/routing/preloading/preloaded.json`)).toBe(true);
Expand Down
2 changes: 1 addition & 1 deletion packages/kit/test/apps/options-2/test/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ test.describe('Service worker', () => {
const response = await request.get('/basepath/service-worker.js');
const content = await response.text();

expect(content).toMatch(/\/_app\/immutable\/entry\/start\.[a-z0-9]+\.mjs/);
expect(content).toMatch(/\/_app\/immutable\/entry\/start\.[a-z0-9]+\.js/);
});

test('does not register /basepath/service-worker.js', async ({ page }) => {
Expand Down
3 changes: 3 additions & 0 deletions packages/kit/test/apps/options/svelte.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,9 @@ const config = {
appDir: '_wheee',
inlineStyleThreshold: 1024,
outDir: '.custom-out-dir',
output: {
preloadStrategy: 'preload-mjs'
},
paths: {
base: '/path-base',
assets: 'https://cdn.example.com/stuff'
Expand Down
14 changes: 14 additions & 0 deletions packages/kit/types/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -422,6 +422,20 @@ export interface KitConfig {
* @default ".svelte-kit"
*/
outDir?: string;
/**
* Options related to the build output format
*/
output?: {
/**
* SvelteKit will preload the JavaScript modules needed for the initial page to avoid import 'waterfalls', resulting in faster application startup. There
* are three strategies with different trade-offs:
* - `modulepreload` - uses `<link rel="modulepreload">`. This delivers the best results in Chromium-based browsers, but is currently ignored by Firefox and Safari (though support is coming to Safari soon).
* - `preload-js` - uses `<link rel="preload">`. Prevents waterfalls in Chromium and Safari, but Chromium will parse each module twice (once as a script, once as a module). Causes modules to be requested twice in Firefox. This is a good setting if you want to maximise performance for users on iOS devices at the cost of a very slight degradation for Chromium users.
* - `preload-mjs` - uses `<link rel="preload">` but with the `.mjs` extension which prevents double-parsing in Chromium. Some static webservers will fail to serve .mjs files with a `Content-Type: application/javascript` header, which will cause your application to break. If that doesn't apply to you, this is the option that will deliver the best performance for the largest number of users, until `modulepreload` is more widely supported.
* @default "modulepreload"
*/
preloadStrategy?: 'modulepreload' | 'preload-js' | 'preload-mjs';
};
paths?: {
/**
* An absolute path that your app's files are served from. This is useful if your files are served from a storage bucket of some kind.
Expand Down
1 change: 1 addition & 0 deletions packages/kit/types/internal.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -337,6 +337,7 @@ export interface SSROptions {
embedded: boolean;
env_public_prefix: string;
hooks: ServerHooks;
preload_strategy: ValidatedConfig['kit']['output']['preloadStrategy'];
root: SSRComponent['default'];
service_worker: boolean;
templates: {
Expand Down