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: avoid using import.meta.url for relative assets if output is not ESM (fixes #9297) #9381

Merged
merged 7 commits into from
Aug 9, 2022

Conversation

sapphi-red
Copy link
Member

@sapphi-red sapphi-red commented Jul 26, 2022

Description

We inject import.meta.url in renderChunk hook when relative base. Rollup replaces them depending on the output module type (ESM, CJS, System, ...). But that replace happens before renderChunk hook (during resolveImportMeta hook).
So if we inject import.meta.url in renderChunk these 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 renderChunk hook)


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • 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.

@sapphi-red sapphi-red added plugin: legacy p4-important Violate documented behavior or significantly improves performance (priority) labels Jul 26, 2022
@sapphi-red
Copy link
Member Author

sapphi-red commented Jul 26, 2022

Instead of this, maybe we should inject a different value for import.meta.url depending on output format. done

@sapphi-red sapphi-red marked this pull request as draft July 26, 2022 17:44
@sapphi-red sapphi-red changed the title fix(legacy): replace import.meta.url with module.meta.url (fixes #9297) fix: avoid using import.meta.url for relative assets if output is not ESM (fixes #9297) Jul 27, 2022
@sapphi-red sapphi-red marked this pull request as ready for review July 27, 2022 13:32
`'file:' + __dirname + '/${relativePath}'`,
`(require('u' + 'rl').URL)`
)} : ${getRelativeUrlFromDocument(relativePath, true)})`
}
Copy link
Member Author

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.

Comment on lines -353 to -357
// TODO: check if this should be removed
if (config.isWorker) {
s = s.replace('import.meta.url', 'self.location.href')
return result()
}
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@sapphi-red sapphi-red Jul 28, 2022

Choose a reason for hiding this comment

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

Do you mean we should use resolveImportMeta?

Copy link
Member

Choose a reason for hiding this comment

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

yep

})

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 () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

The 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)?

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

@sapphi-red sapphi-red linked an issue Aug 5, 2022 that may be closed by this pull request
7 tasks
@sapphi-red sapphi-red mentioned this pull request Aug 9, 2022
9 tasks
Copy link
Member

@patak-dev patak-dev left a comment

Choose a reason for hiding this comment

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

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.

@patak-dev
Copy link
Member

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.

@caoxiemeihao
Copy link
Contributor

caoxiemeihao commented Aug 10, 2022

Hey 👋! @patak-dev Planning the next release? 😄

This PR was not found in 3.0.5

image

@patak-dev
Copy link
Member

patak-dev commented Aug 10, 2022

Yes, we merged this one after 3.0.5, we will do another patch in the next days

@kishore-rajendran
Copy link

@patak-dev is there any timeline when will this fix be released?

@patak-dev
Copy link
Member

Before the end of the week @kishore-rajendran

@kishore-rajendran
Copy link

thanks for the update @patak-dev 😁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p4-important Violate documented behavior or significantly improves performance (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Uncaught TypeError: Failed to construct 'URL': Invalid URL (vite 3) plugin-legacy Unexpected token import
5 participants