fix(worker): using data URLs for inline shared worker #12014

fi3ework · 2023-02-10T09:19:45Z

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 · 2023-02-15T10:40:53Z

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.

poyoho · 2023-02-24T03:34:10Z

docs/config/worker-options.md


 Output format for worker bundle.

+## worker.inlineUrl
+
+- **Type:** `'blob' | 'data'`


It better to rename by base64?

Good naming, updated.

fi3ework · 2023-02-24T13:33:34Z

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

poyoho · 2023-02-24T13:42:53Z

packages/vite/src/node/plugins/worker.ts

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


if setting base64 is not need to createObjectURL

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

fi3ework · 2023-02-24T15:45:59Z

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:

do not allow using blob with SharedWorker
log a warning in userland
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 · 2023-02-24T16:22:13Z

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 · 2023-03-08T18:24:17Z

Update the PR. The behavior would be like this:

If worker.inlineUrl is nullish, inline worker will use blob(fallback to base64) as before, inline sharedworker will use base64 only.
If set worker.inlineUrl to base64, all inline workers will use base64
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.

sapphi-red · 2023-03-12T05:35:15Z

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 · 2023-03-12T15:53:56Z

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

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

sapphi-red · 2023-03-14T02:42:35Z

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.

for inline worker

keep the current behavior, using Blob first and taking data URL as a fallback (data URL for SSR)

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.

sapphi-red · 2023-03-14T02:39:10Z

packages/vite/src/node/plugins/worker.ts

+          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);
+            }


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.

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

sapphi-red

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

packages/vite/src/node/plugins/worker.ts

playground/worker/__tests__/es/es-worker.spec.ts

patak-dev · 2023-03-17T15:32:02Z

packages/vite/src/node/plugins/worker.ts

+          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,


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

Yeah! cleaned.

sapphi-red · 2023-03-18T05:42:31Z

packages/vite/src/node/plugins/worker.ts

+            let objURL;
+            try {
+              objURL = blob && (window.URL || window.webkitURL).createObjectURL(blob);
+              if (objURL) throw ''


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.

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

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.

Co-authored-by: 翠 / green <green@sapphi.red>

poyoho reviewed Feb 24, 2023

View reviewed changes

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

fi3ework force-pushed the inline-url branch from 0f457a8 to 2746454 Compare March 9, 2023 04:12

fi3ework requested a review from poyoho March 9, 2023 04:19

sapphi-red added p3-minor-bug An edge case that only affects very specific usage (priority) feat: web workers labels Mar 12, 2023

fi3ework force-pushed the inline-url branch from 2746454 to f5a4b5d Compare March 12, 2023 17:54

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 reviewed Mar 14, 2023

View reviewed changes

fi3ework force-pushed the inline-url branch 3 times, most recently from bede82c to 7201a46 Compare March 14, 2023 16:42

fi3ework requested a review from sapphi-red March 14, 2023 17:03

sapphi-red requested changes Mar 15, 2023

View reviewed changes

packages/vite/src/node/plugins/worker.ts Outdated Show resolved Hide resolved

sapphi-red reviewed Mar 16, 2023

View reviewed changes

playground/worker/__tests__/es/es-worker.spec.ts Outdated Show resolved Hide resolved

sapphi-red previously approved these changes Mar 16, 2023

View reviewed changes

sapphi-red requested a review from patak-dev March 17, 2023 08:33

patak-dev reviewed Mar 17, 2023

View reviewed changes

fi3ework added 2 commits March 17, 2023 23:44

feat(worker): support inlineUrl of worker config

e03cc1a

refactor: change 'data' to 'base64'

ccc8fd5

fi3ework added 6 commits March 17, 2023 23:44

fix: separate blob and base64 code

6309d34

fix: correct shared worker test

3e12174

test: add more

c5164d2

chore: remove inlineUrl

edfb5a8

fix: try catch createObjectURL fail

0991090

refactor: clean code

a1fd676

fi3ework dismissed sapphi-red’s stale review via a1fd676 March 17, 2023 15:50

fi3ework force-pushed the inline-url branch from 19d68f2 to a1fd676 Compare March 17, 2023 15:50

patak-dev previously approved these changes Mar 17, 2023

View reviewed changes

sapphi-red requested changes Mar 18, 2023

View reviewed changes

fi3ework and others added 2 commits March 18, 2023 15:05

Update packages/vite/src/node/plugins/worker.ts

68a612d

Co-authored-by: 翠 / green <green@sapphi.red>

chore: update test

ec3dc32

fi3ework dismissed patak-dev’s stale review via ec3dc32 March 18, 2023 07:06

fi3ework requested review from sapphi-red and patak-dev March 18, 2023 07:08

sapphi-red approved these changes Mar 18, 2023

View reviewed changes

patak-dev enabled auto-merge (squash) March 18, 2023 07:24

patak-dev merged commit 79a5007 into vitejs:main Mar 18, 2023
11 checks passed

fi3ework deleted the inline-url branch March 18, 2023 13:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(worker): using data URLs for inline shared worker #12014

fix(worker): using data URLs for inline shared worker #12014

fi3ework commented Feb 10, 2023

pragmasoft-ua commented Feb 15, 2023

poyoho Feb 24, 2023

fi3ework Feb 24, 2023

fi3ework commented Feb 24, 2023

poyoho Feb 24, 2023

fi3ework Feb 24, 2023

fi3ework commented Feb 24, 2023 •

edited

pragmasoft-ua commented Feb 24, 2023

fi3ework commented Mar 8, 2023 •

edited

sapphi-red commented Mar 12, 2023

fi3ework commented Mar 12, 2023 •

edited

sapphi-red commented Mar 14, 2023

sapphi-red Mar 14, 2023

fi3ework Mar 14, 2023

sapphi-red left a comment

patak-dev Mar 17, 2023

fi3ework Mar 17, 2023

sapphi-red Mar 18, 2023

fi3ework Mar 18, 2023

patak-dev Mar 18, 2023 •

edited

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

fix(worker): using data URLs for inline shared worker #12014

fix(worker): using data URLs for inline shared worker #12014

Conversation

fi3ework commented Feb 10, 2023

Description

Additional context

What is the purpose of this pull request?

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

pragmasoft-ua commented Feb 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fi3ework commented Feb 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fi3ework commented Feb 24, 2023 • edited

pragmasoft-ua commented Feb 24, 2023

fi3ework commented Mar 8, 2023 • edited

sapphi-red commented Mar 12, 2023

fi3ework commented Mar 12, 2023 • edited

sapphi-red commented Mar 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sapphi-red left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patak-dev Mar 18, 2023 • edited

Choose a reason for hiding this comment

fi3ework commented Feb 24, 2023 •

edited

fi3ework commented Mar 8, 2023 •

edited

fi3ework commented Mar 12, 2023 •

edited

patak-dev Mar 18, 2023 •

edited