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

fix: revert mjs extension usage by default, make it an option #9179

Merged
merged 12 commits into from
Feb 25, 2023
+48 −14
Merged

fix: revert mjs extension usage by default, make it an option #9179

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
9cd88be
fix: revert mjs extension usage by default, make it an option
dummdidumm Feb 23, 2023
df4f24b
fix tests
dummdidumm Feb 23, 2023
e30c2df
reword
dummdidumm Feb 24, 2023
1971dcc
include .mjs files in precompression
dummdidumm Feb 24, 2023
3b738ad
convert to preloadStrategy option
dummdidumm Feb 24, 2023
bb8d8ce
default
dummdidumm Feb 24, 2023
c166198
DRY
Rich-Harris Feb 24, 2023
ccb5599
Update packages/kit/types/index.d.ts
Rich-Harris Feb 24, 2023
9df50dd
fix
Rich-Harris Feb 24, 2023
1a34f86
use modulepreload on basic test
Rich-Harris Feb 24, 2023
083eee8
fix
Rich-Harris Feb 24, 2023
1fdea51
another test
Rich-Harris Feb 24, 2023
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
5 changes: 5 additions & 0 deletions .changeset/grumpy-apricots-admire.md
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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: ${config.kit.output.preloadStrategy},
root,
service_worker: ${has_service_worker},
templates: {
Expand Down
17 changes: 11 additions & 6 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 @@ -516,12 +516,17 @@ 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`,
// see the kit.output.preloadStrategy option for details on why we have multiple options here
entryFileNames: ssr
? '[name].js'
: `${prefix}/[name].[hash]${
kit.output.preloadStrategy === 'preload-mjs' ? '.mjs' : '.js'
}`,
Rich-Harris marked this conversation as resolved.
Show resolved Hide resolved
chunkFileNames: ssr
? 'chunks/[name].js'
: `${prefix}/chunks/[name].[hash]${
kit.output.preloadStrategy === 'preload-mjs' ? '.mjs' : '.js'
}`,
assetFileNames: `${prefix}/assets/[name].[hash][extname]`,
hoistTransitiveImports: false
},
Expand Down
11 changes: 6 additions & 5 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 @@ -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 •
edited
Loading

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
4 changes: 4 additions & 0 deletions packages/kit/test/apps/basics/svelte.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
/** @type {import('@sveltejs/kit').Config} */
const config = {
kit: {
output: {
preloadStrategy: 'preload-mjs'
Copy link
Member

Choose a reason for hiding this comment

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

do we really want to switch our largest test app to a non-default strategy? i'd rather have a separate app test this

Copy link
Member

Choose a reason for hiding this comment

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

yeah, let's move it into options. the tests need updating anyway by the looks of things

Copy link

Choose a reason for hiding this comment

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

The recent bug reports around this could have been avoided if your test suite had a test like this:

  1. Build the example app.
  2. Download the latest nginx Docker image and run it, having it serve the build output.
  3. Run headless Chrome and open the port served by the Nginx container.
  4. Verify that there are no errors in the JavaScript console and that the page has minimal expected behavior (like clicking the button increments the counter).

The Docker orchestration parts could be copied from https://github.com/GeoffreyBooth/js-mjs-mime-type-test/blob/master/test.sh.

Copy link
Member

Choose a reason for hiding this comment

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

That's a huge amount of overhead to guard against a very unlikely regression

Copy link

Choose a reason for hiding this comment

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

That’s a huge amount of overhead to guard against a very unlikely regression

Completely broken sites also seems pretty bad . . . 🤷

You could write the test and just not include it in your suite, then run it whenever you mess with preload stuff. I would think you’d want a test along these lines, loading the site in a real browser, to check that the waterfall doesn’t happen (in the browsers where you’re not expecting it to).

If you’re currently serving the site like via npm run preview and loading it into headless browsers, you could configure whatever Node-based server npm run preview is using the serve the static files to explicitly serve .mjs files without the Content-Type header (or with a wrong one like application/octet-stream). Then you can avoid the overhead of Docker etc. but still have a test that the site works for the large population of “unable to serve .mjs files” environments.

Copy link
Member

Choose a reason for hiding this comment

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

That's simply not going to happen. There are two things we can guard against:

  • re-introducing .mjs, which would require a maintainer to ignore the 'this is why we don't use .mjs' comment that this PR adds
  • every conceivable bug that could happen for every conceivable way someone could use SvelteKit, in which case our test suite would take infinite minutes to run, and we would spend all our lives maintaining the test suite inside of the actual code

Hindsight is 20/20 — it would have been nice not to have introduced this breaking change, but the reality of software development is that breakage happens. That's why we have versions!

},

prerender: {
handleHttpError: 'warn'
},
Expand Down
4 changes: 2 additions & 2 deletions packages/kit/test/apps/options/test/test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,7 @@ test.describe('trailingSlash', () => {
if (process.env.DEV) {
expect(requests.filter((req) => req.endsWith('.svelte')).length).toBe(1);
} else {
expect(requests.filter((req) => req.endsWith('.mjs')).length).toBeGreaterThan(0);
expect(requests.filter((req) => req.endsWith('.js')).length).toBeGreaterThan(0);
}

expect(requests.includes(`/path-base/preloading/preloaded/__data.json`)).toBe(true);
Expand Down Expand Up @@ -262,7 +262,7 @@ test.describe('trailingSlash', () => {
if (process.env.DEV) {
expect(requests.filter((req) => req.endsWith('.svelte')).length).toBe(1);
} else {
expect(requests.filter((req) => req.endsWith('.mjs')).length).toBeGreaterThan(0);
expect(requests.filter((req) => req.endsWith('.js')).length).toBeGreaterThan(0);
}

requests = [];
Expand Down
13 changes: 13 additions & 0 deletions packages/kit/types/index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -422,6 +422,19 @@ export interface KitConfig {
* @default ".svelte-kit"
*/
outDir?: string;
/**
* Options related to the build output format
*/
output?: {
/**
* What preload strategy to use for JavaScript files to avoid waterfalls:
* - `modulepreload` - uses `<link rel="modulepreload">` to preload JavaScript files. Works only in Chromium-based browsers currently, and soon in Safari, too.
* - `preload-js` - uses `<link rel="preload">` to preload JavaScript files. Works in all browsers but Firefox, and causes double-parsing of the script for Chromium-based browser.
* - `preload-mjs` - uses `<link rel="preload">` to preload JavaScript files, but uses the `.mjs` extension. Works in Chromium-based browsers and Safari. Check of your provider/CDN has the correct MIME type for `.mjs` files before turning this on.
Copy link

Choose a reason for hiding this comment

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

Suggested change
* - `preload-mjs` - uses `<link rel="preload">` to preload JavaScript files, but uses the `.mjs` extension. Works in Chromium-based browsers and Safari. Check of your provider/CDN has the correct MIME type for `.mjs` files before turning this on.
* - `preload-mjs` - uses `<link rel="preload">` to preload JavaScript files, but uses the `.mjs` extension. Works in Chromium-based browsers and Safari. Check if your server or CDN sets the correct `Content-Type` header for `.mjs` files (text/javascript) before turning this on; browsers will error on a missing or incorrect `Content-Type` header.

Thank you, this overall comment is much clearer.

* @default "modulepreload"
*/
Rich-Harris marked this conversation as resolved.
Show resolved Hide resolved
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
View file
Open in desktop
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