search

djipco / webmidi

Tame the Web MIDI API. Send and receive MIDI messages with ease. Control instruments with user-friendly functions (playNote, sendPitchBend, etc.). React to MIDI input with simple event listeners (noteon, pitchbend, controlchange, etc.).
Apache License 2.0
1.53k stars 115 forks source link

Option to swap out requestMIDIAccess function when enabling WebMIDI #224

Closed jjeff closed 2 years ago

jjeff commented 2 years ago

Adds a requestMIDIAccessFunction property to the options object argument of WebMIDI.enable(). If this property exists (and is a function), it will be called instead of navigator.requestMIDIAccess. This seems to work in my tests, however I can’t get WebMIDI to build because there seems to be a missing typescript-declarations/generate.js file… so I’m not sure if the unit tests are working correctly for me.

djipco commented 2 years ago

Awesome! Thanks for the PR.

Before I can merge it, there are a few things that need to be adjusted though...

because there seems to be a missing typescript-declarations/generate.js file…

My bad. I renamed the file when I moved from automatic generation to manual. I just committed it back. Are you able to run the tests now?

Speaking of tests, we will need a unit test for that new option. It should go in the enable() section of /test/WebMidi.test.js.

Also, can you copy the modified jsdoc comments to the /typescript/webmidi.d.ts file?

djipco commented 2 years ago

I just saw your updated PR moments ago. So sorry. I'm looking at it now.

djipco commented 2 years ago

Wow! This is a mammoth PR. I will need more time to go through it. I'll get back to you.

jjeff commented 2 years ago

I probably should have commented after I submitted the changes. I actually thought I did... but it looks like I didn't. Sorry about that.

Yeah. This changes all/most of the tests to use the new mock MIDI device. They're not all passing yet... and I'm not sure if it's the tests or the WebMidi library itself. But you would be better than me at determining that.

Let me know if you have any questions.

djipco commented 2 years ago

I don't know if I was supposed to receive a notification from GitHub in this scenario but I'm pretty sure I didn't. Anyway, I am SO sorry I missed your updates. The problem is that, meanwhile, I updated the library. So your PR rolls back some things I changed.

Would it be at all possible for you to submit a new PR plotted against the latest version? Also, would it be possible to separate the changes to the tests from the changes to the library? I am a bit nervous to change the testing model, especially if some tests do not yes pass. Perhaps we could just mark the new requestMIDIAccessFuntion feature as experimental in the documentation until the tests are sorted out?

Again, so sorry about the misunderstanding. I really wish to work with you on this as your PR will be very valuable to the community.

jjeff commented 2 years ago

Yeah. I'll take a look over the weekend and see if I can rebase on the current main and split the tests out into a separate PR.

All good. 👍

jjeff commented 2 years ago

Okay. This PR has been rebased against main. Not a mammoth PR anymore...

However, the Testing PR https://github.com/djipco/webmidi/pull/232 is pretty mammoth and requires/includes this PR. That's where I could use some of your expertise in solving the testing problems.

djipco commented 2 years ago

Awesome! Thank you so much @jjeff.