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): worker import.meta.url should not depends on document in iife mode #12629

Merged
merged 4 commits into from
Apr 2, 2023

Conversation

sun0day
Copy link
Member

@sun0day sun0day commented Mar 28, 2023

Description

fix #12611

import.meta.url will be transformed to something like document.currentScript&&document.currentScript.src||new URL("a.js",document.baseURI).href in iife mode. But document is undefined in the worker context.

This PR directly transforms it to new URL('a.js', self.location.href).href

Additional context


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 PR Title 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.

@stackblitz
Copy link

stackblitz bot commented Mar 28, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@sun0day sun0day changed the title fix(worker): worker import.meta.url should not depends on document fix(worker): worker import.meta.url should not depends on document in iife mode Mar 28, 2023
Copy link

@vinayakkulkarni vinayakkulkarni left a comment

Choose a reason for hiding this comment

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

LGTM

patak-dev
patak-dev previously approved these changes Mar 29, 2023
resolveImportMeta(property, { chunkId, format }) {
// document is undefined in the worker, so we need to avoid it in iife
if (property === 'url' && format === 'iife') {
return `new URL('${chunkId}', self.location.href).href`
Copy link
Member

@sapphi-red sapphi-red Apr 2, 2023

Choose a reason for hiding this comment

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

Suggested change
return `new URL('${chunkId}', self.location.href).href`
return 'self.location.href'

I just noticed that we should use self.location.href here. location.href returns the location of the worker and not the document.
https://developer.mozilla.org/en-US/docs/Web/API/WorkerGlobalScope/location

new URL('${chunkId}', self.location.href).href is be like http://localhost:4173/assets/assets/worker.js which is not correct, because chunkId is like /assets/worker.js and self.location.href is like http://localhost:4173/assets/worker.js.
image

@sapphi-red sapphi-red added p3-minor-bug An edge case that only affects very specific usage (priority) feat: web workers labels Apr 2, 2023
@patak-dev patak-dev merged commit 65f5ed2 into vitejs:main Apr 2, 2023
@vinayakkulkarni
Copy link

Will this go in 4.2.2 ?

@tomayac
Copy link
Contributor

tomayac commented Apr 19, 2023

Will this go in 4.2.2 ?

Seems like it didn't make it. I still get…

var de = document.currentScript && document.currentScript.src || new URL("assets/worker-b4ac415f.js",document.baseURI).href;

…in my code. It works fine in dev mode, but not in preview mode. Hoping for the next release.

@patak-dev
Copy link
Member

@tomayac you can try it out on vite@4.3.0-beta.8. If no regressions are reported, it will be tagged as 4.3 stable tomorrow.

@tomayac
Copy link
Contributor

tomayac commented Apr 19, 2023

Oh, amazing! Confirming that it works fine on 4.3.0-beta.8! Thank you <3!

@tien
Copy link

tien commented May 18, 2023

I'm still getting this issue on 4.3.0-beta.8. Specifically with this dependency @polkadot/api

this line here

export const packageInfo = { name: '@polkadot/x-textdecoder', path: new URL(import.meta.url).pathname, type: 'esm', version: '11.1.3' };

still get compiled to

const yw = {
  name: "@polkadot/x-textdecoder",
  path: {
    url: document.currentScript && document.currentScript.src || new URL("assets/worker-4a3b769e.js",document.baseURI).href
  } && self.location.href ? new URL(self.location.href).pathname.substring(0, new URL(self.location.href).pathname.lastIndexOf("/") + 1) : "auto",
  type: "esm",
  version: "11.1.3"
}

tien added a commit to TalismanSociety/talisman-web that referenced this pull request May 18, 2023
tien added a commit to TalismanSociety/talisman-web that referenced this pull request May 18, 2023
tien added a commit to TalismanSociety/talisman-web that referenced this pull request May 18, 2023
@patak-dev
Copy link
Member

@tien please create a new issue with a minimal reproduction against the latest Vite version (and you can link this PR for reference). A comment on a merged PR doesn't allow us to track your problem properly

dragonsea0927 added a commit to dragonsea0927/talisman-web that referenced this pull request Oct 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: web workers p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Undefined document introduced when using import.meta.url in worker
6 participants