-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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: avoid using import.meta.url
for relative assets if output is not ESM (fixes #9297)
#9381
Changes from all commits
3ca6b49
50baa27
8f2065e
b894e57
1cdb135
da07e4c
4fa7190
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -308,7 +308,7 @@ export function webWorkerPlugin(config: ResolvedConfig): Plugin { | |
} | ||
}, | ||
|
||
renderChunk(code, chunk) { | ||
renderChunk(code, chunk, outputOptions) { | ||
let s: MagicString | ||
const result = () => { | ||
return ( | ||
|
@@ -334,7 +334,8 @@ export function webWorkerPlugin(config: ResolvedConfig): Plugin { | |
'asset', | ||
chunk.fileName, | ||
'js', | ||
config | ||
config, | ||
outputOptions.format | ||
) | ||
const replacementString = | ||
typeof replacement === 'string' | ||
|
@@ -349,12 +350,6 @@ export function webWorkerPlugin(config: ResolvedConfig): Plugin { | |
} | ||
) | ||
} | ||
|
||
// TODO: check if this should be removed | ||
if (config.isWorker) { | ||
s = s.replace('import.meta.url', 'self.location.href') | ||
return result() | ||
} | ||
Comment on lines
-353
to
-357
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. rollup has already replaced There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you mean we should use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yep |
||
} | ||
if (!isWorker) { | ||
const workerMap = workerCache.get(config)! | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,13 +14,19 @@ test('normal', async () => { | |
'worker bundle with plugin success!', | ||
true | ||
) | ||
await untilUpdated( | ||
() => page.textContent('.asset-url'), | ||
isBuild ? '/other-assets/vite' : '/vite.svg', | ||
true | ||
) | ||
}) | ||
|
||
test('TS output', async () => { | ||
await untilUpdated(() => page.textContent('.pong-ts-output'), 'pong', true) | ||
}) | ||
|
||
test('inlined', async () => { | ||
// TODO: inline worker should inline assets | ||
test.skip('inlined', async () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Inside inline worker, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we treat inline workers like lib mode(should we make all the asset to base64)? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we can. But I'm not sure how this should be handled. Also this will need much more work and since this wasn't working before this PR, I think we shouldn't include the change in this PR. |
||
await untilUpdated(() => page.textContent('.pong-inline'), 'pong', true) | ||
}) | ||
|
||
|
@@ -65,7 +71,7 @@ describe.runIf(isBuild)('build', () => { | |
) | ||
|
||
// worker should have all imports resolved and no exports | ||
expect(workerContent).not.toMatch(`import`) | ||
expect(workerContent).not.toMatch(/import(?!\.)/) // accept import.meta.url | ||
expect(workerContent).not.toMatch(`export`) | ||
// chunk | ||
expect(content).toMatch(`new Worker(""+new URL("../worker-entries/`) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,13 +1,14 @@ | ||
import { msg as msgFromDep } from 'dep-to-optimize' | ||
import { mode, msg } from './modules/workerImport' | ||
import { bundleWithPlugin } from './modules/test-plugin' | ||
import viteSvg from './vite.svg' | ||
|
||
self.onmessage = (e) => { | ||
if (e.data === 'ping') { | ||
self.postMessage({ msg, mode, bundleWithPlugin }) | ||
self.postMessage({ msg, mode, bundleWithPlugin, viteSvg }) | ||
} | ||
} | ||
self.postMessage({ msg, mode, bundleWithPlugin, msgFromDep }) | ||
self.postMessage({ msg, mode, bundleWithPlugin, msgFromDep, viteSvg }) | ||
|
||
// for sourcemap | ||
console.log('my-worker.js') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming here that common variables(
require
,module
) are not renamed when using this function.