feat: read sec-fetch-dest header to detect JS in transform

[jonaskuske]

Description

fix #9963

Additional context

---

What is the purpose of this pull request?

• Bug fix
• New Feature
• Documentation update
• Other

Before submitting the PR, please make sure you do the following

• Read the Contributing Guidelines.
• Read the Pull Request Guidelines and follow the Commit Convention.
• Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
• Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
• Ideally, include relevant tests that fail without this PR but pass with it.

[patak-dev]

The problem is that known sources of JS are used in other places, and not only in this condition. I think we should try to resurrect @dominikg's #3828 so @haikyuu can add imba to the list in a vite-plugin-imba. We could also propose to add temporarily imba to the current harcoded list (you can search for astro in the codebase), but it doesn't scale to add more values.

[jonaskuske]

Hmm, @dominikg's PR was in response to #2829, which discusses various issues with dependency scanning and bundling for non-Vue/Svelte/Astro SFCs. That looks like a different beast to me, and also requires changes for the html detection regex and the SFC script extraction logic as described in the first comment there.

The problem is that known sources of JS are used in other places, and not only in this condition.

From a quick search through the Vite source, knownJsSrcRE is only used as part of isJSRequest, which in turn is only used in three places.

Overview of isJSRequest usage sites

1. As one of the conditions to check whether a file should be transformed (subject of this PR)

This is what's causing problems with script[src] in dev mode, as the request comes directly from the HTML where Vite doesn't inject the import query (2), so to Vite the request for script[src] looks like any other request and is not transformed. This would be solved by reading sec-fetch-dest. (or alternatively by appending the ?import query to script[src] in the HTML middleware)
In build mode the problem of #9963 doesn't exist, as Vite reads the HTML and doesn't need file extensions or headers to know that the content of script[src] is, well, a script.

2. As part of isExplicitImportRequired/markExplicitImport to make sure imports not recognized as JS/CSS are transformed

This is what allows Vite to work with unknown script variants already, for everything besides script[src] entrypoints, as the ?import query is appended to import URLs (but not script[type=module] URLs) and ensures the request is transformed by its plugin.

3. To print a warning about JSX requiring a .jsx/.tsx file

Not relevant.

Due to 2. (all imports going through markExplicitImport), loading unknown JS files via imports works already. Unknown JS files in script entries in dev mode are the only part causing errors, so unless I'm missing something, reading sec-fetch-dest or also appending the ?import query in the HTML src is the missing piece that allows configuration-free support (dependency discovery and bundling aside) for JS variants of all kind.
(I'd also suggest renaming isJSRequest to the more accurate isJSUrl though)

[jonaskuske]

Hmm, after thinking this through, running the src attribute of HTML module script elements through markExplicitImport (which only marks if the resource isn't a known JS/CSS one) sounds like the more straight-forward solution to this specific problem 🤔
WDYT?

[patak-dev]

Hmm, after thinking this through, running the src of module scripts through markExplicitImport sounds like the more straight-forward solution to this specific problem 🤔 WDYT?

If I understand you correctly, you want us to start marking every known JS source with ?import, both in <script> and when doing an import in a module. This is too noisy (and may break a lot of the ecosystem), so we should keep the current scheme for known JS source clean. We could automatically mark with ?import anything that isn't a known JS source when it is in a script, since it will also be marked with ?import when imported in a module. But this could still make .imba a second-class citizen for other plugins. Given the current scheme, I think we should allow configuration like @dominikg proposed. Or we should find a better way to differentiate an image imported from a module from an image referenced in CSS or HTML. I'm thinking if we shouldn't go the other way around and only start adding ?import to known non-JS files (as in the list of known asset types here: https://github.com/vitejs/vite/blob/main/packages/vite/src/node/constants.ts#L91). cc @sapphi-red since you have also been looking into ?import related issues lately.

[jonaskuske]

If I understand you correctly, you want us to start marking every known JS source with ?import, both in <script> and when doing an import in a module

No, I just want to append ?import to unknown JS URLs in src attributes, the same way they're appended to import statements. So automatically change this:

<script type="module" src="./entry.myjsdialect">

to this:

<script type="module" src="./entry.myjsdialect?import">

to make sure that these to behave the same way:

<script type="module" src="./entry.myjsdialect"></script>
<!-- and -->
<script type="module">import "./entry.myjsdialect"</script>

Currently, the first variant breaks because the entry file will not be transformed by plugins, but the second variant works. That's inconsistent behavior imo.

So the change would be to run the src attr of a script[type=module] through markExplicitImport, which then checks if the URL is a known JS or CSS resource, and if not, adds the import query. Same thing that's already happening for import statements, just not for HTML script imports.

[patak-dev]

If I understand you correctly, you want us to start marking every known JS source with ?import, both in <script> and when doing an import in a module

No, I just want to append ?import to unknown JS URLs in src attributes, the same way they're appended to import statements.

This is a stretch of the word import, but it makes sense to me as a temporal patch. I think we can do it and work to expose a configuration option or change the scheme in other PRs

[jonaskuske]

Cool, gonna update the PR later! 👍

And yeah, thinking of an entry src as an import first sounded strange to me too. But it kind of makes sense when you consider that it should be equivalent to an actual import statement in an inline module script — and MDN calls it an import, too:

First of all, you need to include type="module" in the <script> element, to declare this script as a module. To import the main.js script, we use this:

<script type="module" src="main.js"></script>

[sapphi-red]

Re Sec-Fetch-Dest, I agree this is better than using ?import to differentiate JS and non-JS requests. But I'm concerned about limiting browsers during dev. We currently support Safari 13, but Sec-Fetch-Dest only works in nightly builds. So I feel it's too early to adopt it.

About appending ?import to src, if this is going to be implemented by transformIndexHtml, it won't work with background integration.

So I think there's three ways here:

1. Give up <script type="module" src="./entry.myjsdialect"> working with background integration and implement appending ?import to Vite
2. Force plugin authors to:
   • append ?import for .myjsdialect by resolveId
   • set Content-Type: application/javascript header for .myjsdialect
3. Force plugin authors to append ?import for .myjsdialect by resolveId and make Vite add Content-Type: application/javascript header when resolvedId has ?import (I'm not sure about this. Maybe this works?)

While writing this comment, I came up with the third one and I feel it's the best approach for now.

[jonaskuske]

Re Sec-Fetch-Dest, I agree this is better than using ?import to differentiate JS and non-JS requests. But I'm concerned about limiting browsers during dev. We currently support Safari 13, but Sec-Fetch-Dest only works in nightly builds. So I feel it's too early to adopt it.

Agreed, browser support is not quite there yet. But fwiw, the indexHtmlMiddleware is already reading Sec-Fetch-Dest:

vite/packages/vite/src/node/server/middlewares/indexHtml.ts

Line 291 in 9f8d79b

if (url?.endsWith('.html') && req.headers['sec-fetch-dest'] !== 'script') {

But yeah, resolveId sounds like a good solution! Not sure if there are other imports that break if we automatically add the JS Content-Type, I'll experiment a bit later :)

[haikyuu]

I will test this tomorrow and see if it works.
Awesome work by the way 💯

[jonaskuske]

This seems to only replace the src with the compiled js file which breaks things

That must be an error on your side, can't reproduce.
(are you returning the transformed code in the resolveId() hook? that needs to stay in transform()!)

However it does indeed not work – resolveId is called as part of transformRequest, so it's guarded by the very same condition that's causing all this. resolveId would need to append the ?import to the id before that transform pipeline begins, but can't, because it's part of that pipeline itself:

if (importQueryExists(id)) {
  if (id.endsWith('.imba')) addImportQuery() // ← never reached! :(
  transform()
}

[haikyuu]

I guess we can add imba to the hardcoded list and work on a proper mechanism to allow plugin developers to declare their extensions

[sapphi-red]

However it does indeed not work – resolveId is called as part of transformRequest, so it's guarded by the very same condition that's causing all this.

That's a good point, I totally forgot that...
So the current workaround is to use configureServer:

configureServer(server) {
  server.middlewares.use((req, res, next) => {
    if (req.url.endsWith('.imba')) {
      req.url += '?import'
      res.setHeader('Content-Type', 'application/javascript')
    }

    next()
  })
},

[haikyuu]

@sapphi-red That's great. That actually works, thank you :) https://stackblitz.com/edit/vitejs-vite-gmkyeg?file=vite.config.js

I'll now make a new version of the plugin.

Should we document this in the plugins section? or do you think it's worth adding as an option to the plugin api?

[haikyuu]

After some testing, I found a little tweak to @sapphi-red's solution in order to support query params that are added sometimes:

import url from 'url'
// ...
                      server.middlewares.use((req, res, next) => {
					const pathname = url.parse(req.url).pathname;
					if (pathname.endsWith('.imba')) {
						req.url += '?import';
						res.setHeader('Content-Type', 'application/javascript');
					}

					next();
				});
			},

[haikyuu]

But that seems to break other things. I have a couple of failing tests now that are complaining about this

[vite] Internal server error: Cannot set properties of undefined (setting 'isSelfAccepting')
  Plugin: vite:import-analysis
  File: /Users/abdellah/workspace/scrimba/imba/temp/serve/vite-env/src/app.imba?import
      at ModuleGraph.updateModuleInfo (file:///Users/abdellah/workspace/scrimba/vite/packages/vite/src/node/server/moduleGraph.ts:144:5)
      at TransformContext.transform (file:///Users/abdellah/workspace/scrimba/vite/packages/vite/src/node/plugins/importAnalysis.ts:693:9)
      at Object.transform (file:///Users/abdellah/workspace/scrimba/vite/packages/vite/src/node/server/pluginContainer.ts:669:11)
      at loadAndTransform (file:///Users/abdellah/workspace/scrimba/vite/packages/vite/dist/node/chunks/dep-1a9f46b8.js:4731:29)

https://github.com/vitejs/vite/blob/main/packages/vite/src/node/server/moduleGraph.ts#L144

[patak-dev]

@haikyuu you may want to use a utility like injectQuery

vite/packages/vite/src/node/utils.ts

Line 307 in 9f8d79b

export function injectQuery(url: string, queryToInject: string): string {

, you need to keep the other queries.

Note: Maybe we could expose injectQuery as an util in the vite package, it seems something that a lot of plugins would need to re-implement

[sapphi-red]

Safari 16.4 Beta added support for Fetch Metadata Request Headers. So in a year, I guess we can start using Sec-Fetch-Dest.

[espipj]

is there any workaround in the meantime? could this be otherwise merged? there's another solution in: #3828 which could be good too...

[jonaskuske]

Again cautiously noting that Vite does already use sec-fetch-dest (#9981 (comment)) – so at least in that case, an improvement that is non-breaking and works in every browser besides one was deemed okay. And that was even before Safari supported sec-fetch-dest. Safari has official support now, so in the case of this fix, every browser is compatible and it's only older Safari versions that don't benefit.

[sapphi-red]

We discussed in the last meeting whether we can merge this in Vite 5.
We concluded that we should wait for the next major for the following reason:

It's been only half a year since Safari started supporting Sec-Fetch-Dest. The version of Safari is tied to the version of macOS, and we suppose that there are a certain number of users who cannot update their OS version due to corporate asset management.
We considered merging this with leaving the isJSRequest(url) || isImportRequest(url) as-is. But that means we'll still need to maintain the knownJsSrcRE list and others. Also it makes the behavior inconsistent among old Safari and others.

Re the fact Vite already uses Sec-Fetch-Dest (#9981 (comment)):
I believe browsers not sending Sec-Fetch-Dest header at this line won't cause any problems. Normally, the transformMiddleware handles all script before reaching this middleware, so it's unlikely that Sec-Fetch-Dest === 'script' will occur. Even if it does, the difference will only be the error message in most cases (not found error / syntax error).

[dominikg]

@haikyuu you may want to use a utility like injectQuery

vite/packages/vite/src/node/utils.ts

Line 307 in 9f8d79b

export function injectQuery(url: string, queryToInject: string): string {

, you need to keep the other queries.
Note: Maybe we could expose injectQuery as an util in the vite package, it seems something that a lot of plugins would need to re-implement

i believe we should do even more. just exposing this util and expecting most framework plugins to call configureServer and bring the same update middleware is still duplicating that code over a lot of plugins. Setting up custom extensions is a common need so it should be as easy as defining an array of extensions that transform to js.

[sapphi-red]

/ecosystem-ci run

[vite-ecosystem-ci]

📝 Ran ecosystem CI on a00523c: Open

suite	result	latest scheduled
astro	❌ failure	❌ failure
marko	❌ failure	✅ success
previewjs	❌ failure	✅ success
redwoodjs	❌ failure	❌ failure
remix	❌ failure	❌ failure
sveltekit	❌ failure	❌ failure
vike	❌ failure	❌ failure
vite-plugin-svelte	❌ failure	❌ failure
vitest	❌ failure	❌ failure

✅ analogjs, histoire, ladle, laravel, nuxt, quasar, qwik, rakkas, storybook, unocss, vite-environment-examples, vite-plugin-pwa, vite-plugin-react, vite-plugin-react-swc, vite-plugin-vue, vite-setup-catalogue, vitepress, vuepress