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): using data URLs for inline shared worker #12014

Merged
merged 10 commits into from
Mar 18, 2023

Conversation

fi3ework
Copy link
Member

Description

close #11956
close #12002

Add config.worker.inlineUrl to select URL type from blob or data

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.

@pragmasoft-ua
Copy link

I'd mention that a combination of blob and shared worker does not make sense. Also, if there are valid cases when application has more than one worker, it will be not possible to have different configurations for different workers.


Output format for worker bundle.

## worker.inlineUrl

- **Type:** `'blob' | 'data'`
Copy link
Member

Choose a reason for hiding this comment

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

It better to rename by 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.

Good naming, updated.

@fi3ework
Copy link
Member Author

CI failed randomly, it should pass with a re-run 😅

@@ -287,7 +287,9 @@ export function webWorkerPlugin(config: ResolvedConfig): Plugin {
export default function WorkerWrapper() {
const objURL = blob && (window.URL || window.webkitURL).createObjectURL(blob);
Copy link
Member

Choose a reason for hiding this comment

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

if setting base64 is not need to createObjectURL

Copy link
Member Author

Choose a reason for hiding this comment

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

updated. Separate the blob and base64 code to make it more readable for human.

@fi3ework
Copy link
Member Author

fi3ework commented Feb 24, 2023

I'd mention that a combination of blob and shared worker does not make sense. Also, if there are valid cases when application has more than one worker, it will be not possible to have different configurations for different workers.

@pragmasoft-ua What you mentioned is correct now. Here are several solutions:

  1. do not allow using blob with SharedWorker
  2. log a warning in userland
  3. do nothing like now

Personally, I would prefer 2. But 3 is also acceptable because it's a runtime error that throw in the browser and Vite itself does nothing wrong due to the configuration.

@pragmasoft-ua
Copy link

I'd mention that a combination of blob and shared worker does not make sense. Also, if there are valid cases when application has more than one worker, it will be not possible to have different configurations for different workers.

@pragmasoft-ua What you mentioned is correct now. Here are several solutions:

1. do not allow using `blob` with `SharedWorker`

2. log a warning in userland

3. do nothing like now

Personally, I would prefer 2. But 3 is also acceptable because it's a runtime error that throw in the browser and Vite itself does nothing wrong due to the configuration.

Unfortunately blob shared workers do not cause runtime error. They silently work, but simply aren't shared.

So, it matters to provide build error or warning, but also matters to provide valid defaults, if configuration is not provided.

@fi3ework fi3ework force-pushed the inline-url branch 2 times, most recently from dbe765b to 0f457a8 Compare March 8, 2023 18:18
@fi3ework
Copy link
Member Author

fi3ework commented Mar 8, 2023

Update the PR. The behavior would be like this:

  1. If worker.inlineUrl is nullish, inline worker will use blob(fallback to base64) as before, inline sharedworker will use base64 only.
  2. If set worker.inlineUrl to base64, all inline workers will use base64
  3. If set worker.inlineUrl to blob, all inline workers will use blob. But if there's a inlined sharedworker, a warning will be thrown in build time.

Additionally, the sharedworker tests're updated. The sharedworker are really dependents on multiple workers to trigger postMessage now which means two sharedworkers will be started to run which was one before. That could help us to ensure if sharedworker works in real world.

@fi3ework fi3ework requested a review from poyoho March 9, 2023 04:19
@sapphi-red sapphi-red added p3-minor-bug An edge case that only affects very specific usage (priority) feat: web workers labels Mar 12, 2023
@sapphi-red
Copy link
Member

Thanks for the PR!

But I don't think we need a config for this.

Given that the blob type inline worker is just for Safari (#3468), I think we can try data-url type inline worker first and fallback to blob type inline worker. But we should not fallback to blob type worker for SharedWorker.

related discussion: #8541 (comment)

@fi3ework
Copy link
Member Author

fi3ework commented Mar 12, 2023

@sapphi-red Thanks for replying.

I think we can try data-url type inline worker first and fallback to blob type inline worker.

In #4674, the data-url was reintroduced for SSR compatibility. However, if we prioritize data-url as the primary option, it is uncertain whether this will cause any issues in userland. Therefore, it may be best to maintain the current behavior. Additionally, I am considering whether or not we should include blob + data fallback logic in the runtime code which could increase its size. 🤔

But we should not fallback to blob type worker for SharedWorker.

It's a good choice.

So assuming we do not add additional config.

  1. for inline worker
    • keep the current behavior, using Blob first and taking data URL as a fallback (data URL for SSR)
  2. for inline shared worker
    • data URL only

@fi3ework fi3ework changed the title feat(worker): support inlineUrl of worker config fix(worker): using data URLs for inline shared worker Mar 12, 2023
@sapphi-red
Copy link
Member

In #4674, the data-url was reintroduced for SSR compatibility. However, if we prioritize data-url as the primary option, it is uncertain whether this will cause any issues in userland. Therefore, it may be best to maintain the current behavior.

So assuming we do not add additional config.

  1. for inline worker

    • keep the current behavior, using Blob first and taking data URL as a fallback (data URL for SSR)
  2. for inline shared worker

    • data URL only

Sounds good to me 👍

Additionally, I am considering whether or not we should include blob + data fallback logic in the runtime code which could increase its size. 🤔

Given that the fallback logic code isn't not much large, I think it's fine.

Comment on lines 301 to 314
export default function WorkerWrapper() {
const objURL = blob && (window.URL || window.webkitURL).createObjectURL(blob);
try {
return objURL ? new ${workerConstructor}(objURL) : new ${workerConstructor}("data:application/javascript;base64," + encodedJs${workerOptions});
} finally {
objURL && (window.URL || window.webkitURL).revokeObjectURL(objURL);
}
Copy link
Member

Choose a reason for hiding this comment

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

try {
  try {
    if (!objURL) throw ''
    return new ${workerConstructor}(objURL)
  } catch {
    return new ${workerConstructor}("data:application/javascript;base64," + encodedJs${workerOptions});
  }
} finally {
  objURL && (window.URL || window.webkitURL).revokeObjectURL(objURL);
}

I think we need a try-catch like this to support #12002. I haven't tested whether an error is thrown in this case though.

The current code doesn't fallback to data-uri worker when the blob worker throwed an error.

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 still could not find reproduce it but I found a similar question https://stackoverflow.com/questions/11420846/whats-wrong-with-my-code-to-record-audio-in-html5. It points to createObjectURL(stream) throwing the error so I put that statement into the try-block.
Also, I think using one try-catch block is enough here, I made a tweak based on your suggestions. Take a look. 😀

packages/vite/src/node/plugins/worker.ts Outdated Show resolved Hide resolved
@fi3ework fi3ework force-pushed the inline-url branch 3 times, most recently from bede82c to 7201a46 Compare March 14, 2023 16:42
Copy link
Member

@sapphi-red sapphi-red left a comment

Choose a reason for hiding this comment

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

I tested with this CSP reproduction: #12002 (comment) and ensured this works. 👍

packages/vite/src/node/plugins/worker.ts Outdated Show resolved Hide resolved
sapphi-red
sapphi-red previously approved these changes Mar 16, 2023
Comment on lines 299 to 322
const blobCode = `${encodedJs}
const blob = typeof window !== "undefined" && window.Blob && new Blob([atob(encodedJs)], { type: "text/javascript;charset=utf-8" });
export default function WorkerWrapper() {
let objURL;
try {
objURL = blob && (window.URL || window.webkitURL).createObjectURL(blob);
if (!objURL) throw ''
return new ${workerConstructor}(objURL)
} catch(e) {
return new ${workerConstructor}("data:application/javascript;base64," + encodedJs${workerOptions});
} finally {
objURL && (window.URL || window.webkitURL).revokeObjectURL(objURL);
}
}`

const base64Code = `${encodedJs}
export default function WorkerWrapper() {
return new ${workerConstructor}("data:application/javascript;base64," + encodedJs${workerOptions});
}
`

return {
// Using blob URL for SharedWorker results in multiple instances of a same worker
code: workerConstructor === 'Worker' ? blobCode : base64Code,
Copy link
Member

Choose a reason for hiding this comment

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

Should we avoid creating the string if they aren't going to be used? Maybe directly move the template strings inside the ternary condition?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah! cleaned.

patak-dev
patak-dev previously approved these changes Mar 17, 2023
let objURL;
try {
objURL = blob && (window.URL || window.webkitURL).createObjectURL(blob);
if (objURL) throw ''
Copy link
Member

Choose a reason for hiding this comment

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

This part has been mistakenly reverted. (also the test case)

I guess you used git push --force after the rebase. FYI there's --force-with-lease flag that can prevent these mistakes happening.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, my bad, had cherry-picked the commits back. Thanks for the careful review. 😆

Copy link
Member

@patak-dev patak-dev Mar 18, 2023

Choose a reason for hiding this comment

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

Uh, thanks @sapphi-red! Sorry, I thought it was a direct refactor. It would be good later to expand the test cases so CI would have failed on this one. Edit, ah, I see the test was also reverted, nvm.

fi3ework and others added 2 commits March 18, 2023 15:05
@patak-dev patak-dev enabled auto-merge (squash) March 18, 2023 07:24
@patak-dev patak-dev merged commit 79a5007 into vitejs:main Mar 18, 2023
@fi3ework fi3ework deleted the inline-url branch March 18, 2023 13:05
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
5 participants