fix: revert mjs extension usage by default, make it an option #9179
Conversation
|
The purist in me wants two changesets for this, one (a fix) rolling back the previous change, and another (a feature) adding the flag. |
|
Do we need to add it to https://kit.svelte.dev/docs/configuration or is that auto-generated? |
|
Autogenerated |
|
Rolling it back will hurt performance for the vast majority of cases where I recognise that there are semver arguments in favour of rolling it back, but I think it's worth considering impact. |
|
I'd stick with |
There's nuance here though. Suppose I'm not wedded to one position or the other but the waterfall on mobile really does suck. |
|
Yeah. I don't know if @dummdidumm can we also sneak in a fix to include |
|
Maybe we should see if we can remove the |
|
In my opinion this very much is a breaking change for the static adapter for webservers that do not recognize the And as for performance - having an option you can turn on for using |
|
To be clear, I was suggesting only serving |
|
Oh, then I misread that, sorry! |
Wasn't this the case until 4-ish days ago? If it was okay until then, I would think it should be okay once again now, at least until a better solution is found. I understand the reluctance to roll back a performance gain, but it's not like the new speed has become so widely used in the last few days that users will react negatively to rolling it back.
Unfortunately I don’t think it’s anywhere close to 99% of apps or users. It’s about the servers serving the files, and static webservers are updated on a very long timeline; Apache 2.2 was end-of-life in 2018, but as of 2023-02-23 it’s still used by 3.5% of all websites. Even the current version of Nginx still doesn’t serve .mjs with the correct Content-Type by default, nor does the current version of Apache httpd, even after all these years. Together, Nginx and Apache account for 66.7% of all webservers. I help manage modules for Node.js, and I’ve been studying this issue for a long time. In 2019 I made a test suite for checking the I found #9018 (comment), and I must say, I’m very surprised to see that code in Chrome. It feels a lot like a spec violation. As far as I know, browsers aren’t supposed to read “file extensions” (really, the last part of the |
| @@ -1,6 +1,10 @@ | |||
| /** @type {import('@sveltejs/kit').Config} */ | |||
| const config = { | |||
| kit: { | |||
| output: { | |||
| preloadStrategy: 'preload-mjs' | |||
There was a problem hiding this comment.
do we really want to switch our largest test app to a non-default strategy? i'd rather have a separate app test this
There was a problem hiding this comment.
yeah, let's move it into options. the tests need updating anyway by the looks of things
There was a problem hiding this comment.
The recent bug reports around this could have been avoided if your test suite had a test like this:
- Build the example app.
- Download the latest
nginxDocker image and run it, having it serve the build output. - Run headless Chrome and open the port served by the Nginx container.
- 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.
There was a problem hiding this comment.
That's a huge amount of overhead to guard against a very unlikely regression
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
| 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') { |
There was a problem hiding this comment.
hmm, is the rel=modulepreload link always added ? shouldn't it be either one or the other?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Not sure I follow? we previously added modulepreload links to the HTTP header and still do (and should)
There was a problem hiding this comment.
as for the 'link headers being too large' thing, that's why we introduced the preload option to resolve
packages/kit/types/index.d.ts
Outdated
| * What preload strategy to use for JavaScript files to avoid waterfalls: | ||
| * - `modulepreload` (default) - 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. |
There was a problem hiding this comment.
| * - `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.
fixes #9141
hides the
.mjsstuff behind an option which isfalseby default (else it's a breaking change, strictly speaking). I choseoutput.mjsas the option name. If we keep that, it may make sense to moveoutDir(top level currently) into that for version 2.Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm testand lint the project withpnpm lintandpnpm checkChangesets
pnpm changesetand following the prompts. Changesets that add features should beminorand those that fix bugs should bepatch. Please prefix changeset messages withfeat:,fix:, orchore:.