-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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
Conversation
There was a problem hiding this 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?
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)); |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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);
Using revokeObjectURL right after just like modderme123 stated, seems to do the job. Tested on iOS and Chrome. I wrapped it in a |
@Xerios Looks like the tests are now failing, can you take a look? |
Forgot about the minifier, used another piece of code to match against. |
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?
Before submitting the PR, please make sure you do the following
fixes #123
).