fix: avoid using import.meta.url for relative assets if output is not ESM (fixes #9297)
#9381
Conversation
sapphi-red
added
plugin: legacy
p4-important
|
|
sapphi-red
changed the title
import.meta.url with module.meta.url (fixes #9297)import.meta.url for relative assets if output is not ESM (fixes #9297)
| `'file:' + __dirname + '/${relativePath}'`, | ||
| `(require('u' + 'rl').URL)` | ||
| )} : ${getRelativeUrlFromDocument(relativePath, true)})` | ||
| } |
There was a problem hiding this comment.
I'm assuming here that common variables(require, module) are not renamed when using this function.
| // TODO: check if this should be removed | ||
| if (config.isWorker) { | ||
| s = s.replace('import.meta.url', 'self.location.href') | ||
| return result() | ||
| } |
There was a problem hiding this comment.
rollup has already replaced import.meta.url with corresponding value when we hit here.
Maybe we need to use /import\.meta\.url/g instead of 'import.meta.url' and do this in transform or resolveImportMeta?
It would work even if import.meta.url is preserved when using module worker. But it wouldn't work if we didn't replace import.meta.url with self.location.href when using classic worker because rollup replaces it with document.currentScript && document.currentScript.src || document.baseURI.
There was a problem hiding this comment.
Maybe we need to use /import.meta.url/g instead of 'import.meta.url' and do this in transform or resolveImportMeta?
use resolveImportMeta hook to rewrite it.
There was a problem hiding this comment.
Do you mean we should use resolveImportMeta?
| }) | ||
|
|
||
| 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.
Inside inline worker, self.location.href and import.meta.url are both blob:foobar or data:foobar. So new URL('./foo', import.meta.url) will become invalid URL.
Do we need to inline assets for inline workers? Or do we need to show an error for them? Or pass the current url into them?
There was a problem hiding this comment.
Can we treat inline workers like lib mode(should we make all the asset to base64)?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I missed this PR before. Fantastic work here @sapphi-red, I totally missed that the replacement was happening before renderChunk. I hope that with Rollup 3, we may be able to drop some of our custom assets handling and use the infra that Rollup already has in place to avoid duplicating so much complexity.
|
I think this is important enough to get in a patch, we can test with this and other related PRs with vite-ecosystem-ci before moving forward. |
|
Hey 👋! @patak-dev Planning the next release? 😄 This PR was not found in
|
|
Yes, we merged this one after 3.0.5, we will do another patch in the next days |
|
@patak-dev is there any timeline when will this fix be released? |
|
Before the end of the week @kishore-rajendran |
|
thanks for the update @patak-dev 😁 |
Description
We inject
import.meta.urlinrenderChunkhook when relative base. Rollup replaces them depending on the output module type (ESM, CJS, System, ...). But that replace happens beforerenderChunkhook (duringresolveImportMetahook).So if we inject
import.meta.urlinrenderChunkthese won't be replaced by Rollup.This PR changes the injected value depending on the output module type.
fixes #9297
Additional context
Related rollup implementation: rollup/rollup#2164
Related rollup hook: https://rollupjs.org/guide/en/#resolveimportmeta (this is called before
renderChunkhook)What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123).