search

ircam-ismm / node-web-audio-api

Web Audio API implementation for Node.js
https://www.npmjs.com/package/node-web-audio-api
BSD 3-Clause "New" or "Revised" License
96 stars 13 forks source link

Feat - Implement proper inheritance chain #99

Closed b-ma closed 6 months ago

b-ma commented 6 months ago

Review the architecture to use facades instead of mixins to better follow spec inheritance (should fix #97)

main

RESULTS:
  - # pass: 5406
  - # fail: 569
  - # type error issues: 33

feat/inheritance-chain

RESULTS:
  - # pass: 5415
  - # fail: 565
  - # type error issues: 30

@jampekka could you confirm it works for your use-case? If it does having a simple example would be really welcome!

jampekka commented 6 months ago

Whoa, that was fast! Per quick look seems very nice. I'm very busy today, OK if I review and test tomorrow?

b-ma commented 6 months ago

No problem, let me know!

jampekka commented 6 months ago

Can confirm that checking for AudioNode parent works for my case. However, AudioNode is not exported, so I have to dig it out by const AudioNode = Object.getPrototypeOf(GainNode). I'll try if I can manage to fix this.

Sorry for the delay with this. I had some problems getting the building working (just stupid user errors).

jampekka commented 6 months ago

Works with AudioNode.connect monkeypatching too. See #101 and https://github.com/jampekka/node-web-audio-api/blob/199c5ee5556f3b09a011c4d8f821fe6cabc4aa0b/examples/composite-audio-node.mjs

Not sure if this should be an example though. Perhaps rather a test? Regarding those, npm test doesn't work currently, changing the test script to mocha tests does fix this.

P.S. Is making a pull request against this pull request branch the proper way to suggest code changes in review?

b-ma commented 6 months ago

Not sure if this should be an example though.

I think it's nice to have as an example, I will have a test with another approach which I think could be a bit more clean (i.e. extending GainNode or something), but as it's proposed by google dev its nice to have here anyway

Regarding those, npm test doesn't work currently, changing the test script to mocha tests does fix this.

Yup, I always run them explicitely npm test -- tests/*.spec.js, maybe should be added in README ?

b-ma commented 6 months ago

All flags green :) Thanks

jampekka commented 6 months ago

I think it's nice to have as an example, I will have a test with another approach which I think could be a bit more clean (i.e. extending GainNode or something), but as it's proposed by google dev its nice to have here anyway

Extending GainNode should work. But for composition I think you still have to hack the .connect somehow. The Google dev proposal is from 2016, when the spec (or implementations) maybe didn't support AudioNode constructors, so there may be cleaner ways now, although I didn't find one.

Yup, I always run them explicitely npm test -- tests/*.spec.js, maybe should be added in README ?

Would be helpful for new contributors. What's the reason not to have this as the npm test script? Does that e.g. get automatically run by some toolchain but we don't want to run them?

(I think you mean npm test -- tests/*.spec.mjs)

b-ma commented 6 months ago

Extending GainNode should work. But for composition I think you still have to hack the .connect somehow. The Google dev proposal is from 2016, when the spec (or implementations) maybe didn't support AudioNode constructors, so there may be cleaner ways now, although I didn't find one.

I've never really tried myself, but that's something I'm quite interested in for creating audio libs, that's the occasion :)

Would be helpful for new contributors. What's the reason not to have this as the npm test script? Does that e.g. get automatically run by some toolchain but we don't want to run them?

Don't really know, probably just that I'm used to it... let's do that, this is more simple, we can always call mocha directly if needed