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 relative base should use import.meta.url #9204

Merged
merged 2 commits into from Jul 22, 2022

Conversation

patak-dev
Copy link
Member

@patak-dev patak-dev commented Jul 18, 2022

Description

Fixes bug report 2. at:

From that comment:

When building with base: './', workers are instantiated with new Worker('../../path/to/worker.js'), rather than with a root-relative path beginning with base. But that's wrong — the path will be relative to the document, not the module instantiating the worker. I think it should be something like this:

new Worker(new URL('../../path/to/worker.js', import.meta.url).href)

What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

@patak-dev patak-dev added the p3-downstream-blocker 🔨 Blocking the downstream ecosystem to work properly (priority) label Jul 18, 2022
return {
runtime: `new URL(${JSON.stringify(
path.posix.relative(path.dirname(importer), filename)
)},import.meta.url).href`
Copy link
Contributor

@tony19 tony19 Jul 20, 2022

Choose a reason for hiding this comment

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

Nit:

Suggested change
)},import.meta.url).href`
)}, import.meta.url).href`

Copy link
Contributor

@tony19 tony19 Jul 20, 2022

Choose a reason for hiding this comment

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

Oh. Is the return value supposed to be minified output, where that space is intentionally omitted?

Copy link
Member Author

@patak-dev patak-dev Jul 20, 2022

Choose a reason for hiding this comment

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

Yes, but I think we could add the space. It will be minified later 👍🏼

Copy link
Member Author

@patak-dev patak-dev Jul 20, 2022

Choose a reason for hiding this comment

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

Actually we should review all these if we do in a separate PR (there are several snippets like this in the code base)

Copy link
Member

@bluwy bluwy left a comment

Other than @tony19's comment of the console.log, this looks good to me

Co-authored-by: Tony Trinh <tony19@gmail.com>
bluwy
bluwy approved these changes Jul 20, 2022
@patak-dev patak-dev merged commit 0358b04 into main Jul 22, 2022
16 of 18 checks passed
@patak-dev patak-dev deleted the fix/worker-relative-base branch Jul 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-downstream-blocker 🔨 Blocking the downstream ecosystem to work properly (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants