vite
vite copied to clipboard
fix: read sec-fetch-dest to detect JS in transform
Description
fix #9963
Additional context
What is the purpose of this pull request?
- [X] Bug fix
- [ ] New Feature
- [ ] Documentation update
- [ ] Other
Before submitting the PR, please make sure you do the following
- [X] Read the Contributing Guidelines.
- [x] Read the Pull Request Guidelines and follow the Commit Convention.
- [x] Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
- [x] 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.
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 https://github.com/vitejs/vite/pull/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.
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)
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?
Hmm, after thinking this through, running the
src
of module scripts throughmarkExplicitImport
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
If I understand you correctly, you want us to start marking every known JS source with ?import, both in
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.
If I understand you correctly, you want us to start marking every known JS source with ?import, both in
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
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 themain.js
script, we use this:<script type="module" src="main.js"></script>
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:
- Give up
<script type="module" src="./entry.myjsdialect">
working with background integration and implement appending?import
to Vite - Force plugin authors to:
- append
?import
for.myjsdialect
byresolveId
- set
Content-Type: application/javascript
header for.myjsdialect
- append
- Force plugin authors to append
?import
for.myjsdialect
byresolveId
and make Vite addContent-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.
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, butSec-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
: https://github.com/vitejs/vite/blob/9f8d79b9897427882bb7da1bdb7862a15938c865/packages/vite/src/node/server/middlewares/indexHtml.ts#L291
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 :)
I will test this tomorrow and see if it works. Awesome work by the way 💯
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()
}
I guess we can add imba to the hardcoded list and work on a proper mechanism to allow plugin developers to declare their extensions
However it does indeed not work –
resolveId
is called as part oftransformRequest
, 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()
})
},
@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?
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();
});
},
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
@haikyuu you may want to use a utility like injectQuery
https://github.com/vitejs/vite/blob/9f8d79b9897427882bb7da1bdb7862a15938c865/packages/vite/src/node/utils.ts#L307, 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