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

fix(worker): handle self reference url worker in dependency for build #17846

Merged
merged 4 commits into from
Aug 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion packages/vite/src/node/plugins/worker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -292,7 +292,7 @@ export function webWorkerPlugin(config: ResolvedConfig): Plugin {

let urlCode: string
if (isBuild) {
if (isWorker && this.getModuleInfo(cleanUrl(id))?.isEntry) {
if (isWorker && config.bundleChain.at(-1) === cleanUrl(id)) {
urlCode = 'self.location.href'
} else if (inlineRE.test(id)) {
const chunk = await bundleWorkerEntry(config, id)
Expand Down
2 changes: 1 addition & 1 deletion packages/vite/src/node/plugins/workerImportMetaUrl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ export function workerImportMetaUrlPlugin(config: ResolvedConfig): Plugin {
if (
isBuild &&
config.isWorker &&
this.getModuleInfo(cleanUrl(file))?.isEntry
config.bundleChain.at(-1) === cleanUrl(file)
) {
s.update(expStart, expEnd, 'self.location.href')
} else {
Expand Down
2 changes: 1 addition & 1 deletion playground/worker/__tests__/es/worker-es.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ describe.runIf(isBuild)('build', () => {
test('inlined code generation', async () => {
const assetsDir = path.resolve(testDir, 'dist/es/assets')
const files = fs.readdirSync(assetsDir)
expect(files.length).toBe(34)
expect(files.length).toBe(35)
const index = files.find((f) => f.includes('main-module'))
const content = fs.readFileSync(path.resolve(assetsDir, index), 'utf-8')
const worker = files.find((f) => f.includes('my-worker'))
Expand Down
8 changes: 7 additions & 1 deletion playground/worker/__tests__/iife/worker-iife.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ describe.runIf(isBuild)('build', () => {
test('inlined code generation', async () => {
const assetsDir = path.resolve(testDir, 'dist/iife/assets')
const files = fs.readdirSync(assetsDir)
expect(files.length).toBe(22)
expect(files.length).toBe(23)
const index = files.find((f) => f.includes('main-module'))
const content = fs.readFileSync(path.resolve(assetsDir, index), 'utf-8')
const worker = files.find((f) => f.includes('worker_entry-my-worker'))
Expand Down Expand Up @@ -173,6 +173,12 @@ test('self reference url worker', async () => {
)
})

test('self reference url worker in dependency', async () => {
await expectWithRetry(() =>
page.textContent('.self-reference-url-worker-dep'),
).toBe('pong: main\npong: nested\n')
})

test.runIf(isServe)('sourcemap boundary', async () => {
const response = page.waitForResponse(/my-worker.ts\?worker_file&type=module/)
await page.goto(viteTestUrl)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ describe.runIf(isBuild)('build', () => {

const files = fs.readdirSync(assetsDir)
// should have 2 worker chunk
expect(files.length).toBe(44)
expect(files.length).toBe(46)
const index = files.find((f) => f.includes('main-module'))
const content = fs.readFileSync(path.resolve(assetsDir, index), 'utf-8')
const indexSourcemap = getSourceMapUrl(content)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ describe.runIf(isBuild)('build', () => {

const files = fs.readdirSync(assetsDir)
// should have 2 worker chunk
expect(files.length).toBe(22)
expect(files.length).toBe(23)
const index = files.find((f) => f.includes('main-module'))
const content = fs.readFileSync(path.resolve(assetsDir, index), 'utf-8')
const indexSourcemap = getSourceMapUrl(content)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ describe.runIf(isBuild)('build', () => {
const assetsDir = path.resolve(testDir, 'dist/iife-sourcemap/assets')
const files = fs.readdirSync(assetsDir)
// should have 2 worker chunk
expect(files.length).toBe(44)
expect(files.length).toBe(46)
const index = files.find((f) => f.includes('main-module'))
const content = fs.readFileSync(path.resolve(assetsDir, index), 'utf-8')
const indexSourcemap = getSourceMapUrl(content)
Expand Down
9 changes: 9 additions & 0 deletions playground/worker/dep-self-reference-url-worker/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
export function startWorker(handler) {
const worker = new Worker(new URL('./worker.js', import.meta.url), {
Copy link

Choose a reason for hiding this comment

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

Would it be possible to also allow ./index.js here? I.e. a self reference from the main script? This is how emscripten starts its pthread workers. All workers (including the main page) use the same script.

Copy link
Collaborator Author

@hi-ogawa hi-ogawa Nov 11, 2024

Choose a reason for hiding this comment

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

I think that should work too though one caveat is that Vite needs to bundle index.js twice for main script and as worker script. So, it's likely that some js might got duplicated but IIRC static wasm assets can be de-duped.

type: 'module',
})
worker.postMessage('main')
worker.addEventListener('message', (e) => {
handler(e)
})
}
7 changes: 7 additions & 0 deletions playground/worker/dep-self-reference-url-worker/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"name": "@vitejs/test-dep-self-reference-url-worker",
"private": true,
"version": "1.0.0",
"type": "module",
"main": "index.js"
}
14 changes: 14 additions & 0 deletions playground/worker/dep-self-reference-url-worker/worker.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
// copy of playground/worker/self-reference-url-worker.js
self.addEventListener('message', (e) => {
if (e.data === 'main') {
const selfWorker = new Worker(new URL('./worker.js', import.meta.url), {
Copy link

Choose a reason for hiding this comment

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

In emscripten we use the self-references a lot (to start pthread workers).

Would it be possible to allow the shorter form of this which is the even more explicit self reference: new URL(import.meta.url). Its also more portable to write this shorter form since its robust against the containing file getting renamed.

Copy link
Collaborator Author

@hi-ogawa hi-ogawa Nov 11, 2024

Choose a reason for hiding this comment

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

Yeah, that should work as Vite would skip analysis and don't have to rewrite it.
What Vite does is to detect recursive worker of this form new URL("./some-file", import.meta.url) and rewrite it into new URL(self.location.href), so the runtime can reference it easily.
So any of these should work:

  • new Worker(import.meta.url)
  • new Worker(new URL(import.meta.url))
  • new Worker(self.location.href)

I confirmed it on my repro, but if you see something different, please submit a separate issue to track it.

type: 'module',
})
selfWorker.postMessage('nested')
selfWorker.addEventListener('message', (e) => {
self.postMessage(e.data)
})
}

self.postMessage(`pong: ${e.data}`)
})
5 changes: 5 additions & 0 deletions playground/worker/index.html
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,11 @@ <h2 class="format-iife">format iife:</h2>
</p>
<code class="self-reference-url-worker"></code>

<p>
<span class="classname">.self-reference-url-worker-dep</span>
</p>
<code class="self-reference-url-worker-dep"></code>

<p>
new Worker(new URL('../deeply-nested-worker.js', import.meta.url), { type:
'module' })
Expand Down
1 change: 1 addition & 0 deletions playground/worker/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
"debug": "node --inspect-brk ../../packages/vite/bin/vite"
},
"dependencies": {
"@vitejs/test-dep-self-reference-url-worker": "file:./dep-self-reference-url-worker",
"@vitejs/test-dep-to-optimize": "file:./dep-to-optimize"
}
}
3 changes: 3 additions & 0 deletions playground/worker/vite.config-iife.js
Original file line number Diff line number Diff line change
Expand Up @@ -51,4 +51,7 @@ export default defineConfig({
},
],
cacheDir: 'node_modules/.vite-iife',
optimizeDeps: {
exclude: ['@vitejs/test-dep-self-reference-url-worker'],
},
})
6 changes: 6 additions & 0 deletions playground/worker/worker/main-module.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import * as depSelfReferenceUrlWorker from '@vitejs/test-dep-self-reference-url-worker'
import myWorker from '../my-worker.ts?worker'
import InlineWorker from '../my-worker.ts?worker&inline'
import InlineSharedWorker from '../my-inline-shared-worker?sharedworker&inline'
Expand Down Expand Up @@ -174,3 +175,8 @@ selfReferenceUrlWorker.addEventListener('message', (e) => {
document.querySelector('.self-reference-url-worker').textContent +=
`${e.data}\n`
})

depSelfReferenceUrlWorker.startWorker((e) => {
document.querySelector('.self-reference-url-worker-dep').textContent +=
`${e.data}\n`
})
10 changes: 10 additions & 0 deletions pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.