dszakallas / mixxx-launchpad

Novation Launchpad mapping for Mixxx
MIT License
32 stars 4 forks source link

Shipping with future versions of Mixxx #58

Closed Swiftb0y closed 1 month ago

Swiftb0y commented 2 years ago

Hey there. I'm one of the members of the Mixxx core team. I'm one of the few people focusing on our hardware mapping facilities. I've just seen that you still care to maintain this mapping, which is rare for most controller mappings. In the next upcoming Mixxx version, we will have upgraded the JS engine to a newer implementation that supports the ES7 spec. We also want to eliminate generated external code in the long run, so while we would love to merge updates to this mapping, we very much want to avoid complex build processes which require lots of setup and build-time dependencies. As already mentioned, the transpilation into ancient ES3 should become unnecessary in Mixxx 2.4. Unfortunately, the code still has flow-typings which have to be removed as part of a build process before the code would run in a vanilla JS environment. These flow-typings could be rewritten as JSDoc comments.

What do you think? I've seen you filed #42 so I understand if you dislike the proposal of removing the build process. Thanks.

Swiftb0y commented 2 years ago

Note that there are some caveats. We only upgraded the environment itself, we still have the XML document for registering handlers and ES6 modules won't work in 2.4 as well. So a build process is still acceptable in the short term for working around these issues. We're planning to address these problems in the long-term.

dszakallas commented 2 years ago

Hi @Swiftb0y, with an ES7 compatible engine, we could remove all library dependencies except eventemitter. Eventemitter is not a large dependency, we could include it in the source code. Also, after removing Flow (I do not object to this, Flow did not become very popular), the remaining benefits for the transpiler/bundler is

I think that switching back to common js modules would be a step in the wrong direction. I would happily simplify the build process by updating the target platform to ES7, so that we could remove all runtime transformations and also remove lodash from the dependencies.

Swiftb0y commented 2 years ago

Great, thank you so much. As I said, because of the mentioned shortcomings, a build process is still justified.

support for multiple controllers. If we have a module system where code sharing is not an issue, the only extra build process we require is generating the XML file for each controller.

This issue is intended to be solved in the same step to get module support working. We have not worked out the details, but it is likely that we'll have a separate API for registering handlers, or as an escape hatch, just forward the midi data directly to the mapping. In either way declaring all handlers upfront in XML would not be needed anymore. Distinguishing between different the different devices of the launchpad family should also be possible (by matching USB vendor and product IDs for example).

Note that no-one is actively working on these newer mapping features though, the only current guaranteed development is that Mixxx 2.4 will ship with a ES7 compatible engine (apart from ES6 module support).

Also, since you already built these mappings in a strictly-typed manner, we would very welcome if those types would stay preserved by translating the flow annotations into JSDoc. We're planning to make the Mixxx API and libraries more typed as well in the future.

dszakallas commented 1 year ago

@Swiftb0y I am creating a new major release soon. It drops support for Mixxx<2.3 as it uses engine.makeConnection. I also rewrote the whole thing to use Typescript and dropped lodash, and instead use babel with corejs to polyfill ES features. See #69

Swiftb0y commented 1 year ago

Thank you. Highly appreciated. Unfortunately we did not have time to work on our ESModule-based engine. So if you want to use ES7, you'll have to target mixxx 2.4 and transpile each mapping to its own module-free file (as you already did previously). If you want to target mixxx 2.3, you'll have to transpile to ES3 instead. Targetting 2.2 wouldn't make sense anyways since we dropped support for that long ago.

If your new release is ready, I'd highly appreciate if you could open a PR with the new mapping files.

Swiftb0y commented 10 months ago

Hey there, I was just working on https://github.com/mixxxdj/mixxx/issues/12369 and I came across your autogenerated launchpad scripts. TLDR; the semantics of engine.beginTimer have to change, the callback no longer automatically has its this rebound to the this of the calling context of the engine.beginTimer call. Since the launchpad mappings are autogenerated and the code is quite high-level, I wasn't comfortable touching those to fix the issue. Instead I'd just wanted to let you know about this change so you can keep in mind for the next mapping version targeted at 2.4 or higher. I'd highly appreciate if you could go through the code in question and verify this doesn't break anything. Thank you.

dszakallas commented 10 months ago

Hi,

nothing required for this script, as I use arrow functions, and those are always bound to the enclosing this. (As a matter of fact, the arrows are transpiled to normal functions, but the babel transform takes care of any this inside by replacing them with a dummy variable set to the enclosing this).

I took a brief look at the referenced issue. I find the new behavior of not rebinding this better as it is less magic and let's this work like ordinary JS. Also, if the function is on a prototype of some object, it might like to use itsthis instead. For the use case in the ticket, you should use arrow or bind the function to the enclosing this explicitly.

Swiftb0y commented 10 months ago

Yes thank you for your assessment of the situation. I agree with all of your points. Not rebinding this is definitely better, but still a breaking API change as I said, so I just wanted to inform you about it. It's good to know that the transpilation takes care of it automatically.