2 pull requests

Asynchronous MIDI library for Node.js and HTML

2 pull requests

Postby platywompus » Wed Nov 29, 2017 11:43 am

Hey Sema,

I'm just starting to dive into your JZZ.js code for a piano learning project and I'm very impressed. I'll admit that even after a few days I'm still a novice, but I can appreciate the power and complexity in the architecture.

I have run into 2 minor problems that I was able to get around with some code tweaks and thought I'd share them. Before that, I'll admit that I may just be misunderstanding a concept so take these suggestions with a grain of salt :)

I've also submitted them via github, but wanted a chance to list out my thoughts in more depth.

1) possible for...in loop conflict (.hasOwnProperty() tweak) pull req
Although I generally subscribe to the "simpler is better" js philosophy and avoid flashy ES6/node.js stuff in favor of just writing the damn code, my project has required I use some open source ES6 code (and have had to use babel to transpile it to ES5). This has required me to add the popular Array.prototype.find polyfill to get IE support (see
https://github.com/jsPolyfill/Array.prototype.find and https://developer.mozilla.org/en-US/doc ... Array/find).

This works by adding a .find() prototype to all array-type objects, but it breaks in the JZZ.js code on the for...in statements which are used regularly to execute chains of functions (now they try to execute the .prototype.find() method as well). By adding a .hasOwnProperty() check in those for...in loops I was able to get JZZ.js to play nice with this prototype but still call all the user added method chains.

This change seems low risk and may help future proof JZZ.js as these sorts of polyfills and prototypes are becoming increasingly common.


2) closing and reopening ports pull req
One requirement i have is I want people to be able to open many pages of my learning site at once in different tabs (if they see multiple scores they want to practice on later) and have midi-in "just work" when they change tabs while practicing. Since the midi-in locks seem to be exclusive i wanted to be able to close ports on window.blur, then open them again on window.focus.

This tweak is small, and seems to work correctly, but I'm less sure about it as I haven't fully grasped the JZZ.js architecture. I found that currently closing then re-opening ports doesn't seem to work, but if I added logic to the port close function to also remove the port from the "_engine._inMap" array that they seem to support re-opening.

Though it seems to work and is a small tweak, it's possible I'm missing a greater implication, so i'm less sure you should accept this change.


Again, thanks for all the work! After spending a few days in the JZZ.js code base, the scope is humbling, but I appreciate it even more.

Cheers,

platy
platywompus
 
Posts: 4
Joined: Sun Nov 19, 2017 12:57 pm

Re: 2 pull requests

Postby sema » Wed Nov 29, 2017 10:09 pm

Wow! Thanks a lot for your contribution!
The changes look reasonable, but I think I need a little more time to have a closer look...
sema
Site Admin
 
Posts: 328
Joined: Mon Oct 17, 2011 7:28 pm

Re: 2 pull requests

Postby sema » Thu Nov 30, 2017 8:03 pm

Done! Thanks again Michael!
sema
Site Admin
 
Posts: 328
Joined: Mon Oct 17, 2011 7:28 pm

Re: 2 pull requests

Postby platywompus » Fri Dec 01, 2017 8:37 am

Glad to help :D your tools (jzz, jazz-soft, jazz-midi) make my whole learning app possible! Its really encouraging to see projects like this pushing the limits of browsers.

Cheers
platywompus
 
Posts: 4
Joined: Sun Nov 19, 2017 12:57 pm


Return to JZZ.js

Who is online

Users browsing this forum: No registered users and 1 guest

cron