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

Unexpected content if there's import() in a js file and import with ?url #11266

Closed
7 tasks done
rwv opened this issue Dec 8, 2022 · 17 comments · Fixed by #14918
Closed
7 tasks done

Unexpected content if there's import() in a js file and import with ?url #11266

rwv opened this issue Dec 8, 2022 · 17 comments · Fixed by #14918
Labels
feat: web workers p3-minor-bug An edge case that only affects very specific usage (priority)

Comments

@rwv
Copy link

rwv commented Dec 8, 2022

Describe the bug

I import a worker.js's URL using

import WorkerURL from "@/vendors/worker.js?url"

And vite serves worker URL to browser with added file header if there's a import() in the worker file

import { injectQuery as __vite__injectQuery } from "/@vite/client";

// origin worker file contents
// ........ 
// ........

and cause error Uncaught SyntaxError: Cannot use import statement outside a module

Reproduction

https://github.com/rwv/vite-import-url-issue

Steps to reproduce

Run npm run dev and npm run build and open the web page.

System Info

System:
    OS: Linux 5.4 Ubuntu 20.04.1 LTS (Focal Fossa)
    CPU: (16) x64 Intel(R) Xeon(R) CPU E5-2680 0 @ 2.70GHz
    Memory: 24.60 GB / 31.40 GB
    Container: Yes
    Shell: 5.8 - /usr/bin/zsh
  Binaries:
    Node: 16.15.1 - ~/.nvm/versions/node/v16.15.1/bin/node
    Yarn: 1.22.19 - ~/.nvm/versions/node/v16.15.1/bin/yarn
    npm: 8.11.0 - ~/.nvm/versions/node/v16.15.1/bin/npm

Used Package Manager

npm

Logs

No response

Validations

@rwv
Copy link
Author

rwv commented Dec 8, 2022

P.S.
In debugging, I trigger a wired mode where

import WorkerURL from "@/vendors/worker.js?url"
// WorkerURL = "@fs/src/vendors/worker.js"

instead of

import WorkerURL from "@/vendors/worker.js?url"
// WorkerURL = "/src/vendors/worker.js"

and things work but I am unable to reproduce it.

@rwv
Copy link
Author

rwv commented Dec 8, 2022

Related: #5690

@luo3house
Copy link
Contributor

Enable module for worker.

const worker = new Worker(WorkerJSURL, {
+  type: "module"
});

MDN: https://developer.mozilla.org/en-US/docs/Web/API/Worker/Worker#syntax
Also please notice browser compatibility.

@rwv
Copy link
Author

rwv commented Dec 9, 2022

The worker creation code is emscripten generated and I cannot modify it. Otherwise i could use the following code to create worker.

import Worker from './woker.js?worker'
const worker = new Worker()

From doc, user will expect the ?url import will import js file without modification. Another use case is that user create a script tag and add Subresource Integrity (SRI) attr, this will cause script tag to fail.

Also the behavior is different between npm run dev and npm run build which cause confusion.

@rwv
Copy link
Author

rwv commented Dec 9, 2022

A possible fix way:
For those static assets imported with ?url, add a prefix @url/. Therefore vite can treat js and js?url differently.

@luo3house
Copy link
Contributor

I am sorry for misunderstanding. I guest you gonna to keep workers splited out of main chunk. Just like this PR prevents assets bundling inline?

@rwv
Copy link
Author

rwv commented Dec 9, 2022

I am sorry for misunderstanding. I guest you gonna to keep workers splited out of main chunk. Just like this PR prevents assets bundling inline?

I do want to keep workers split out of main chunk. But I don’t think this is related to that PR.
The problem is when I import a js file using ?url, I want to import the original file as a url. However, when the browser fetch this url, vite don’t know this is a ?url imported file and assume it is a regular js file which is imported by other files and therefore compile this file.

P.S that PR is related to build stage. However this issue only happens in npm run dev

P.P.S In other word, vite cannot distinguish a js file is imported by

import test from "test.js"

Or

import test from "test.js?url"

@jamsinclair
Copy link
Contributor

I think this is an unintended bug. There's a lot of edge cases and this would have been easy to miss.

The import analysis plugin needs additional heuristics for when its in a classic worker and to use importScripts in this situation instead.

@bluwy bluwy added p3-minor-bug An edge case that only affects very specific usage (priority) feat: web workers and removed pending triage labels Aug 21, 2023
@bluwy
Copy link
Member

bluwy commented Aug 21, 2023

Somewhat related to #12995, maybe we can inline the injectQuery instead of importing it.

@jamsinclair
Copy link
Contributor

@bluwy that looks amazing! I was halfway through writing a similar solution, I should've checked first 😅 . I think inlining the function is the way to go for sure 👍

@rwv
Copy link
Author

rwv commented Aug 21, 2023

Somewhat related to #12995, maybe we can inline the injectQuery instead of importing it.

I don’t think inline the injectQuery is a good idea. We should make ?url imported js file as-is otherwise it may break subresource integrity(SRI)

@rwv
Copy link
Author

rwv commented Aug 21, 2023

Also when a user use ?url to import js file, this user expects this js file unmodified otherwise this user will import js file directly.

@bluwy
Copy link
Member

bluwy commented Aug 21, 2023

@rwv in Vite, if you import the file regardless of ?url, it will process and transform the file. So not just the import and inline injection, but optimizations like minifying is included. If you don't want these features, you should put the file in the public directory instead.

@rwv
Copy link
Author

rwv commented Aug 21, 2023

My workaround is to put worker file in public folder. However ?url includes multiple advantages like filename hashing. And since vite provide static file ?url importing, and this feature can be used by common user. We should keep ?url functionality accurate.

@bluwy
Copy link
Member

bluwy commented Aug 21, 2023

We can't change the behaviour of ?url today to not process it. Putting it in the public directory would be the supported method here. If you need hashing you can write a Vite plugin to hash it manually.

@Adriaaaaan
Copy link

I'm tearing my hair out trying to get this to work. Same scenario. I've already put the worker js file in the assets folder but still vite insists on adding that annoying import. why??
Any more detail on the workaround?

@rwv
Copy link
Author

rwv commented Nov 9, 2023

Thank you for your awesome PR! 🎉
However this PR still has a problem as vite sill modifies the content of .js?url import file. As a result, it mismatch the semantics of ?url import (import file as-is) and will break functionalities like SRI.

@github-actions github-actions bot locked and limited conversation to collaborators Nov 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feat: web workers p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants