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

Loading script to AudioWorklet #1093

Closed
ronkot opened this issue Mar 29, 2018 · 34 comments · Fixed by #6495
Closed

Loading script to AudioWorklet #1093

ronkot opened this issue Mar 29, 2018 · 34 comments · Fixed by #6495

Comments

@ronkot
Copy link

ronkot commented Mar 29, 2018

❓Question:

I'm trying to use new AudioWorklet feature (enabled by default in Chrome 66 ->). Part of the process is to load a WorkletProcessorScript to a web worker from my main js file like this:

...
audioContext.audioWorklet.addModule('my-worklet-processor.js').then(() => {
  ...
});

So I'd need to keep my-worklet-processor.js as a separate file. Maybe this is already possible, but I can't find a way to do this. Any clues how to solve this? Big thanks! 🙏

There's not yet good documentation on AudioWorklets, but here and here are some examples I've been using.

🌍 Your Environment

Software Version(s)
Parcel 1.4.1
Node 8.9.4
npm/Yarn npm 5.7.1
Operating System OSX 10.11.6
@mischnic
Copy link
Member

Maybe it needs to be added to

const WORKER_RE = /\bnew\s*Worker\s*\(/;
to be treated like a web worker.

@mischnic
Copy link
Member

mischnic commented Mar 29, 2018

@ronkot What happens currently? Does 'my-worklet-processor.js' get replaced with something else?

@ronkot
Copy link
Author

ronkot commented Mar 29, 2018

@mischnic my-worklet-processor.js does not seem to get replaced (if I understood your question correctly). File is requested as http://localhost:1234/js/my-worklet-processor.js (running in dev mode), which in turn returns the content of my index.html.

@mischnic
Copy link
Member

This correctly detects the calls, but the emitted worklet file contains the hmr runtime.

diff --git a/src/assets/JSAsset.js b/src/assets/JSAsset.js
index 7261c35..6fffbb5 100644
--- a/src/assets/JSAsset.js
+++ b/src/assets/JSAsset.js
@@ -16,7 +16,7 @@ const IMPORT_RE = /\b(?:import\b|export\b|require\s*\()/;
 const GLOBAL_RE = /\b(?:process|__dirname|__filename|global|Buffer)\b/;
 const FS_RE = /\breadFileSync\b/;
 const SW_RE = /\bnavigator\s*\.\s*serviceWorker\s*\.\s*register\s*\(/;
-const WORKER_RE = /\bnew\s*Worker\s*\(/;
+const WORKER_RE = /\bnew\s*Worker\s*\(|audioWorklet\.addModule\s*\(/;

 class JSAsset extends Asset {
   constructor(name, pkg, options) {
diff --git a/src/visitors/dependencies.js b/src/visitors/dependencies.js
index e697db7..4652f81 100644
--- a/src/visitors/dependencies.js
+++ b/src/visitors/dependencies.js
@@ -61,6 +61,13 @@ module.exports = {
       return;
     }

+    if(types.isStringLiteral(args[0]) && types.isMemberExpression(callee) &&
+       callee.object.property && callee.object.property.name === "audioWorklet" &&
+       callee.property.name === "addModule") {
+      addURLDependency(asset, args[0]);
+      return;
+    }
+
     const isRegisterServiceWorker =
       types.isStringLiteral(args[0]) &&
       matchesPattern(callee, serviceWorkerPattern);

@ronkot
Copy link
Author

ronkot commented Apr 6, 2018

Any news on this? Could I help somehow - by providing a minimal working example for example?

Also, I'd be happy to hear if there's some temporary hack that I could use.

@habemus-papadum
Copy link

I would be interested in some basic guidance about this as well.

My thoughts are for now to inline the script as multiline string, convert it to a Blob, and use the Blob's url in addModule.

This looses all the potential transpilation wins parcel provides but since these scripts are meant to be lean and mean, they probably need a tailored transpilation approach anyway (ala glslify, perhaps)

@habemus-papadum
Copy link

@mischnic Using your patches suggested above, does the hmr runtime added to the Audio Worklet script cause to script to fail, or is the hmr code functional in the AudioWorklet context (which doesn't seem all that bad...) ? (Or has this not been tested -- in which case I can try)

@mischnic
Copy link
Member

mischnic commented Apr 3, 2019

does the hmr runtime added to the Audio Worklet script cause to script to fail

I don't recall the exact error.

is the hmr code functional in the AudioWorklet context

HMR won't work because you can register a AudioWorkletProcessor only once:

If the name exists as a key in the node name to processor definition map, throw a NotSupportedError exception and abort these steps because registering a definition with a duplicated key is not allowed. (https://webaudio.github.io/web-audio-api/#dom-audioworkletglobalscope-registerprocessor)

@habemus-papadum
Copy link

@mischnic Thanks! all makes sense

@quasicomputational
Copy link

This'd be solved by #2306. If you want to try a hackish workaround today, you might try my parcel-plugin-import-as-url.

@mischnic
Copy link
Member

mischnic commented Apr 4, 2019

This'd be solved by 2306

That's not needed for this, normal WebWorkers work fine with Parcel.

@H-s-O
Copy link

H-s-O commented Feb 19, 2020

Any news on this ?

@mischnic
Copy link
Member

Now, there are some more worklet types with CSS Houdini

@ProLoser
Copy link

ProLoser commented Jan 30, 2021

@mischnic could either implicitly detect *worklet.addModule or could just let people deal with this themselves explicitly by doing some sort of import worklet from 'worklet:./some/file.js' and this way you don't have to deal with all the edge cases around the syntax.

@mischnic
Copy link
Member

mischnic commented Jan 30, 2021

audioWorklet.addModule is very easy to add, it's essentially the same as new Worker at the moment.
The only difference to webworkers is that worklets don't support importScripts, but only ESM.
(and we'd want to only use the syntax here: #5430 for worklets and then deprecated the current new Worker("url") syntax)

A pipeline that returns a URL to a worker is harder to do....

@ProLoser
Copy link

ProLoser commented Jan 30, 2021

@mischnic so if I understand things correctly, your suggested fix works as long as I use --no-hmr for serve and it shouldn't be an issue for bundle, correct? I already have HMR disabled due to worklet problems, this would be an acceptable compromise for me.

@mischnic
Copy link
Member

mischnic commented Jan 30, 2021

Yes (so the approach from #1093 (comment) but for v2 and using #5430)

@ProLoser
Copy link

ProLoser commented Jan 30, 2021

Does this look correct?

// @parcel/transformer-js/src/visitors/dependencies.js
      if (types.isStringLiteral(args[0]) && types.isMemberExpression(callee) &&
        callee.object.property && callee.object.property.name === "audioWorklet" &&
        callee.property.name === "addModule") {

        addURLDependency(asset, ast, args[0], {
          env: {
            context: 'worker',
            outputFormat:
              isModule && asset.env.scopeHoist ? 'esmodule' : undefined,
          },
          meta: {
            webworker: true,
          },
        });
        return;
      }

Pinging @dioptre

@mischnic
Copy link
Member

mischnic commented Jan 30, 2021

I thought that audioWorklet was a global variable (like service worker), but apparently not. This makes detecting the call much more problematic...

@ProLoser
Copy link

Can we detect worklet.addmodule( (case-insensitive) or is it complicated due to the AST?

@mischnic
Copy link
Member

mischnic commented Jan 31, 2021

  1. detect worklet.addmodule(?

That is not a problem, but how would you differentiate this from

class Foo {
	addModule(data) {
		...
	}
}
let worklet = new Foo();
worklet.addModule(....)

(= it's not really possible to determine if worklet is actually an AudioContext)

  1. just set isIsolated for these imports in a transformer?

This breaks once you there are shared bundles in the worker, because asset.env.context isn't set to "worker" or something like that.

I also found that adding an import to the worklet results in a worklet.js doesn't export 'default' error.

@ProLoser
Copy link

ProLoser commented Feb 7, 2021

Yes I've seen some other bundler wrap the contents in an export default {code} but wasn't sure why.

Personally, I wish this was a little bit more declarative and explicit, I really like the xyz: pattern as it's clear even if you never seen it before. I believe we should add a worker: transformer that applies the context and bundling correctly. Would that fix things? Do worklets just need to have a default export vs workers?

As long as we had SOME solution for now to be unblocked that is flexible for everyone's weird scenario would be great, we can try to get the automagic working later (and possibly integrate with native apis better if supported)

@ProLoser
Copy link

ProLoser commented Feb 7, 2021

Would that be because the import is not treated as a runtime entrypoint so it's expected to have some sort of export?

@ProLoser
Copy link

ProLoser commented Feb 7, 2021

I think detecting worker.addModule() is totally acceptable: http://developer.mozilla.org/en-US/docs/Web/API/Worklet/addModule

Although I guess some weird obscure library dependency could break :(

I would clearly document it prominently and see who complains lol.

@ProLoser
Copy link

ProLoser commented Feb 7, 2021

I'm testing this out:

// @parcel/transformer-js/src/visitors/dependencies.js
// ...
CallExpression: {
// ...
	let isWorkletAddModule =
        types.isMemberExpression(callee) &&
        types.matchesPattern(callee, 'Worklet.addModule') && // (W
        args.length >= 1 &&
        types.isStringLiteral(args[0]) &&
        !isInFalsyBranch(ancestors);

      if (isWorkletAddModule) {
        let isModule = false;
        if (types.isObjectExpression(args[1])) {
          let prop = args[1].properties.find(v =>
            types.isIdentifier(v.key, { name: 'type' }),
          );
          if (prop && types.isStringLiteral(prop.value))
            isModule = prop.value.value === 'module';
        }

        addURLDependency(asset, ast, args[0], {
          env: {
            context: 'web-worker',
            outputFormat:
              isModule && asset.env.shouldScopeHoist ? 'esmodule' : undefined,
          },
          meta: {
            webworker: true,
          },
        });
        return;
      }
// ...

@mischnic
Copy link
Member

mischnic commented Feb 7, 2021

As I've said, your version of detecting worker.addModule() might work in your case. But it's not safe generally and we certainly can't do this for everyone with the default config unless worker is proven to be an actual native AudioWorklet object.

I've pushed an experiment from some time ago for supporting worker: and worklet: imports: https://github.com/parcel-bundler/parcel/tree/worker-url. The current implementation would certainly break once shared modules between worklets and other contexts are introduced.

@ProLoser
Copy link

ProLoser commented Feb 7, 2021

Oh that's awesome! I understand that reservation. I will play with it, luckily my worklet code is not complex enough to have any dependencies (yet)

@ProLoser
Copy link

ProLoser commented Feb 8, 2021

Okay I've checked out your branch, reinstalled xcode, linked everything, went into my project and linked in the packages you modified in your worker-url branch (core, transformer-worker/worklet, config-default) and built my project. Unfortunately I'm seeing this error:

 ~/S/zeus   master ±  yarn browser:build --no-cache
yarn run v1.22.10
$ yarn workspace browser build --no-cache
$ parcel build ./src/index.html --no-cache
🚨 Build failed.
@parcel/packager-js: esmodule output is not supported without scope hoisting.
Error: esmodule output is not supported without scope hoisting.
    at Object.package (/Users/dsofer/Sites/parcel/packages/packagers/js/lib/JSPackager.js:187:13)
    at PackagerRunner.package (/Users/dsofer/Sites/parcel/packages/core/core/lib/PackagerRunner.js:350:27)
    at async PackagerRunner.getBundleResult (/Users/dsofer/Sites/parcel/packages/core/core/lib/PackagerRunner.js:311:20)
    at async PackagerRunner.getBundleInfo (/Users/dsofer/Sites/parcel/packages/core/core/lib/PackagerRunner.js:303:9)
    at async Child.handleRequest (/Users/dsofer/Sites/parcel/packages/core/workers/lib/child.js:255:9)
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
error Command failed.
Exit code: 1
Command: /Users/dsofer/.nvm/versions/node/v14.15.2/bin/node
Arguments: /usr/local/Cellar/yarn/1.22.10/libexec/lib/cli.js build --no-cache
Directory: /Users/dsofer/Sites/zeus/packages/browser
Output:

info Visit https://yarnpkg.com/en/docs/cli/workspace for documentation about this command.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

In my code I have the following:

import encoderWorker from 'worklet:./encoderWorker.js';

// ...
audioWorklet.addModule(encoderWorker)

@ProLoser
Copy link

@mischnic do I need to link every single parcel package when using this fork? Or just a specific subset?

@mischnic
Copy link
Member

The easiest way would probably be just copying the transformers into your project (if you're using a monorepo anyway), because there aren't any changes to Parcel core anyway.

@ProLoser
Copy link

I'm looking at your diff https://github.com/parcel-bundler/parcel/compare/worker-url it looks like you do modify the core but I'll try copying the packages over

@ProLoser
Copy link

ProLoser commented Feb 14, 2021

It's kinda hard to understand why this is happening, such is the intricacies of a monorepo, any chance this branch could get merged?
image

@ProLoser
Copy link

Parcel keeps trying to reinstall packages when I run it and it's clearing out the copied over packages. I don't really know of a quicker way to implement this other than getting it into a nightly release.

@shaibam
Copy link

shaibam commented Jun 20, 2021

I can't beleive I'm saying this but inlining it worked for me:
https://stackoverflow.com/questions/55503925/can-the-paintworklet-be-inlined

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants