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

Jump to bottom

Add support for other workers/worklets #54

Merged
merged 11 commits into from Nov 18, 2021
Merged

Add support for other workers/worklets #54

merged 11 commits into from Nov 18, 2021

Conversation

hughrawlinson
Copy link
Collaborator

@hughrawlinson hughrawlinson commented Jun 27, 2021 •
edited

Sorry for the delay - here's the beginnings of a PR to address #52.

It works just fine in my own test. But right now it only supports Base64 Audio Worklets. The other things that need support are:

  • Base64
    • Audio Worklet
    • Paint Worklet
    • Service Worker
    • Shared Worker
    • Animation Worklet
    • Layout Worklet
  • Inline
    • Audio Worklet
    • Paint Worklet
    • Service Worker
    • Shared Worker
    • Animation Worklet
    • Layout Worklet
  • URL
    • Audio Worklet
    • Paint Worklet
    • Service Worker
    • Shared Worker
    • Animation Worklet
    • Layout Worklet
  • Documentation

Given how much there is to add support for, I wonder if it's worth changing the structure of the helper directory a little so that there isn't so much riding on the filenames?

A problem that cropped up is that AudioWorklet doesn't exist in Node. I have the helper method throwing an error, but would love to hear your take on what should be done about that. Would it be better to fail during compile time based on the target?

It's also not clear that Paint Worklet and Animation Worklet are widely supported. I'll speak to some people and try to figure out the utility at this point. Once AudioWorklet is in, adding the others should be fairly straightforward based on that example. Web Audio API finally made it to Recommendation status at the W3C last week.

Good news is that I don't think this patch will require a major version bump - I was able to keep the original 'pattern' option around, so that this isn't a breaking change.

I plan to continue working on adding support for the other Worklets in the matrix above - although depending on what you think it might make sense to split this work out into multiple PRs rather than have all in one - and maybe first focus on AudioWorklet?

Also, is there a testing strategy? I would love to provide some tests for this if that's useful - just not really sure how to.

Thanks!

darionco
darionco reviewed Jun 28, 2021
View reviewed changes
src/index.js Outdated Show resolved Hide resolved
@darionco
Copy link
Owner

Nice! I am super excited about this PR!

@hughrawlinson
Copy link
Collaborator Author

Awesome, great to hear! Any thoughts as to the above questions? Also I noticed that #49 is open and adds another worker that I had missed on my list. Shall we add that to the major release?

@hughrawlinson
remove backward compat for pattern option
5a7cca1
@hughrawlinson
add support for paint worklets
7bf3a09
@hughrawlinson
make paintworklet not async
528a2c3
@hughrawlinson
remove scaffolding for animationworklet
59a78be 
animationworklet and layout worklet don't have implementations in any
browsers yet so I'll suggest that we add them as a feature once that
happens.
hughrawlinson
hughrawlinson commented Jul 19, 2021
View reviewed changes
src/helper/browser/createURLPaintWorkletFactory.js Outdated
@@ -0,0 +1,6 @@
export function createURLPaintWorkletFactory(url) {
return async function PaintWorkletFactory(options) {
return await CSS.paintWorklet.addModule(url, options);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So there's a minor issue here. paintWorklet isn't necessarily supported in all browsers, and as such, CSS.paintWorklet may not be defined. We have a few options.

  • Leave code as-is to throw the referenceerror. Pros: we don't have to take an error handling stance. Cons: it might not be clear to the developer what's happening.
  • Catch the error and throw a more explanatory error
  • In the case that it fails, return some other object from the function. Pros: we won't throw runtime exceptions. Cons: it might be unclear to the developer when their paint call in CSS just doesn't work.
  • Log a debug statement rather than throw an error?

Let me know what approach you'd like me to take here.

@hughrawlinson hughrawlinson marked this pull request as ready for review July 19, 2021 13:46
@hughrawlinson
Copy link
Collaborator Author

Ok I think we're close to there, certainly ready for a review. The outstanding questions are:

  • There aren't any tests. I've manually tested as much as I can, but would it be worth getting some automated tests in place?
  • I pulled in @code2933's code for sharedworkers from feat: SharedWorker support #49 since they're another worker type here. LMK if that isn't ok, happy to remove them from this PR.
  • What should we do about throwing errors in node when the worker/worklet type isn't available? And in browsers that don't support for example paintWorklet (IE).
  • I removed AnimationWorklet and LayoutWorklet from the scope of this PR since there are no implementations in any browsers. It should be fairly straightforward to look at this PR and add support for them once/if they do make it to browsers.
  • Are the docs ok? Anything more needs clarifying?
  • Almost definitely a separate PR, but one that I'm happy to do and might fit in with releasing 2.0.0 - how to you feel about adding typescript types? The declaration file in the repo you link to is good, and I could add additional types for the other worklet types. Or, if you want to keep this repo typescript free, we could put types up on DefinitelyTyped.

Let me know if anything else is unclear, of course happy to answer any questions or make any changes.

@hughrawlinson hughrawlinson force-pushed the add-support-for-other-worklets branch from 47e50c0 to 638fe78 Compare July 19, 2021 14:54
@hughrawlinson hughrawlinson changed the title working mvp of base64audioworkletfactory Add support for other workers/worklets Jul 19, 2021
@hughrawlinson hughrawlinson force-pushed the add-support-for-other-worklets branch from 638fe78 to de76260 Compare July 19, 2021 18:45
@hughrawlinson hughrawlinson mentioned this pull request Oct 20, 2021
Open
5 tasks
@darionco
Copy link
Owner

This is so awesome, I'm so sorry it took this long to get to it, life stuff and all that.

Thank you so much for all this work!

@darionco darionco merged commit df25e6a into darionco:master Nov 18, 2021
@darionco
Copy link
Owner

Let me know if you'd be interested on maintaining this project, unfortunately, my current situation will not allow me to continue with development. :(

@hughrawlinson
Copy link
Collaborator Author

Thanks @darionco! I would certainly consider that 😄

Would you be able to publish a new version of the package with this change?

@hughrawlinson hughrawlinson deleted the add-support-for-other-worklets branch November 19, 2021 14:49
@darionco
Copy link
Owner

@hughrawlinson I have invited you as a collaborator for this project, if you give me your npm username I'll add you as collaborator there as well so you can publish new versions of the package.

@hughrawlinson
Copy link
Collaborator Author

Thanks! Same username as here - https://www.npmjs.com/~hughrawlinson 😄

@darionco
Copy link
Owner

Done, thank you for your contributions!

Let me know if I can help in any other way!

@jaimeadf
Copy link

Hey, will these changes get a new version in NPM? I have the latest version 1.6.1 installed, and it only supports web workers.

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@darionco darionco darionco left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@hughrawlinson @darionco @jaimeadf @code2933