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: inline webworker safari support #3468

Merged
merged 5 commits into from
May 23, 2021

Conversation

Xerios
Copy link
Contributor

@Xerios Xerios commented May 19, 2021

Description

Issue related to this #2504

Safari doesn't play by the rules, so we have to adapt.

Base64 isn't accepted for inline webworkers on Safari, yet blobs are fine. This very simple change adapted from my own custom plugin solves this issue. Tested on multiple Tim Apple's devices and non-Tim Apple devices, works perfectly so far and it is currently used in production.

I'm aware that there's #2689 but this change is slimmer and frankly very simple compared to what the hell is going on in that thread.


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.

@Shinigami92 Shinigami92 added the p3-minor-bug An edge case that only affects very specific usage (priority) label May 19, 2021
nihalgonsalves
nihalgonsalves previously approved these changes May 19, 2021
Copy link
Member

@nihalgonsalves nihalgonsalves left a comment

Choose a reason for hiding this comment

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

Nice! @NotWoods @modderme123 any thoughts?

@nihalgonsalves nihalgonsalves linked an issue May 19, 2021 that may be closed by this pull request
@milomg
Copy link
Contributor

milomg commented May 19, 2021

This change seems reasonable and should fix some errors with inline workers. This issue does not make #2689 obsolete. Rather, it complements the work there that fixes non-inline workers in Safari and Firefox


return `export default function WorkerWrapper() {
const blob = new Blob([atob(\"${Buffer.from(output[0].code).toString('base64')}\")], { type: 'text/javascript;charset=utf-8' });
return new Worker((window.URL || window.webkitURL).createObjectURL(blob));
Copy link
Contributor

Choose a reason for hiding this comment

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

createObjectURL creates a pointer to the blob. I believe it can be freed after creating the Worker with revokeObjectURL.

Copy link
Contributor Author

@Xerios Xerios May 19, 2021

Choose a reason for hiding this comment

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

Oh I didn't realize it required some manual memory management. In my project this is completely unnecessary since I only have long-lived webworkers, thus irrelevant to my use case. I guess one change I can do is just move the objectURL outside of that function, so that it doesn't recreate it on multiple calls.

I'm really curious to know how one would solve this issue in an automated way.

Copy link
Contributor

@milomg milomg May 19, 2021

Choose a reason for hiding this comment

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

I believe the suggestion is to do (entirely untested)

const blob = new Blob([atob(\"${Buffer.from(output[0].code).toString('base64')}\")], { type: 'text/javascript;charset=utf-8' }); 
const objURL = (window.URL || window.webkitURL).createObjectURL(blob);
const worker = new Worker(obj);
(window.URL || window.webkitURL).revokeObjectURL(objURL);
return worker;

Edit: fixed the typo mentioned below

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, immediately revoking the URL as shown above will fix the issue.

Slight change in the snippet: Pass the URL to revoke, not the blob

const objURL = (window.URL || window.webkitURL).createObjectURL(blob);
const worker = new Worker(obj);
-(window.URL || window.webkitURL).revokeObjectURL(blob);
+(window.URL || window.webkitURL).revokeObjectURL(objURL);

@Xerios
Copy link
Contributor Author

Xerios commented May 20, 2021

Using revokeObjectURL right after just like modderme123 stated, seems to do the job. Tested on iOS and Chrome.

I wrapped it in a try finally in case Worker fails to initialize ( haven't exactly tested this part, but in theory this should work )

NotWoods
NotWoods previously approved these changes May 21, 2021
nihalgonsalves
nihalgonsalves previously approved these changes May 21, 2021
@nihalgonsalves
Copy link
Member

@Xerios Looks like the tests are now failing, can you take a look?

@Xerios Xerios dismissed stale reviews from nihalgonsalves and NotWoods via 2cfe036 May 21, 2021 13:32
@Xerios
Copy link
Contributor Author

Xerios commented May 21, 2021

Forgot about the minifier, used another piece of code to match against.

@patak-dev patak-dev requested a review from antfu May 21, 2021 19:35
@antfu antfu changed the title Fix inline webworker safari support fix: inline webworker safari support May 23, 2021
@antfu antfu merged commit 2671546 into vitejs:main May 23, 2021
ygj6 pushed a commit to ygj6/vite that referenced this pull request Jun 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

Inline workers fail in safari due to the use of data urls
7 participants