search

Kagami / vmsg

:musical_note: Library for creating voice messages
https://kagami.github.io/vmsg/
Creative Commons Zero v1.0 Universal
348 stars 58 forks source link

refactor to expose core recorder functionality separate from the demo UI #7

Closed transitive-bullshit closed 6 years ago

transitive-bullshit commented 6 years ago

First off, excellent work with this module and writeup. I've learned a lot from reading through both.

This PR makes it possible to use this module's awesome recording functionality without the bundled recorder UI.

I wanted to use just the recording functionality for react-mp3-recorder.

Instead of just exporting the Form class via a record function, it now exports record and Recorder, where the latter is just the core recording functionality without any UX.

Note that I've made sure the accompanying demo 100% still works with these changes. This refactor isn't mean to change the old functionality. I also tried to keep the changes minimal.

My recommendation for a follow-up PR would be to completely remove the UX from this module and just have it in the accompanying demo, as I doubt anyone will really want to use the Form UI as-is. Either that or move the current UX into a separate module like vmsg-dom so the functionality is properly encapsulated from the presentation.

Thanks!

Kagami commented 6 years ago

Thanks! This looks good despite the few nitpiks.

My recommendation for a follow-up PR would be to completely remove the UX from this module and just have it in the accompanying demo, as I doubt anyone will really want to use the Form UI as-is. Either that or move the current UX into a separate module like vmsg-dom so the functionality is properly encapsulated from the presentation.

I was considering this, but came to conclusion it might be easier to restyle default UI with CSS rules. (And if you need some customization, chances are there won't be enough flexibility in how worker handling/audio streams are arranged.) Though now I'm not sure it would work good for everybody.

In my opinion default UI component won't hurt, with tree shaking you don't even get its code. And it's easier to maintain everything in a single module. What do you think?

Kagami commented 6 years ago

This refactor isn't mean to change the old functionality

I noticed one thing that changed though. At first demo requests permissions to use mic, then opens window with progress indicator, then loads worker. It takes some time to load and compile WebAssembly module, so it's better to split init() method of Recorder class into initAudio() and initWorker(). If you wish, you can also export and call the both methods in Recorder#init as a shortcut.

transitive-bullshit commented 6 years ago

it's better to split init() method of Recorder class into initAudio() and initWorker()

Good call -- I split init into initAudio and initWorker as suggested.

In my opinion default UI component won't hurt, with tree shaking you don't even get its code. And it's easier to maintain everything in a single module. What do you think?

With these changes, I agree I think it should be fine as a single module.

transitive-bullshit commented 6 years ago

Github isn't showing me your latest comment, but I think it should be the correct order now :)

Kagami commented 6 years ago

Thanks, merged.