Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Shipping with future versions of Mixxx #58

Open
Swiftb0y opened this issue Jan 25, 2022 · 8 comments
Open

Shipping with future versions of Mixxx #58

Swiftb0y opened this issue Jan 25, 2022 · 8 comments

Comments

@Swiftb0y
Copy link

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
Copy link
Author

Swiftb0y commented Jan 25, 2022

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
Copy link
Owner

dszakallas commented Feb 28, 2022

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

  • support for ES imports (not needed if the engine supports it natively)
  • 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.

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
Copy link
Author

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
Copy link
Owner

dszakallas commented Mar 18, 2023

@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
Copy link
Author

Swiftb0y commented Mar 18, 2023

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
Copy link
Author

Swiftb0y commented Dec 5, 2023

Hey there, I was just working on mixxxdj/mixxx#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
Copy link
Owner

dszakallas commented Dec 10, 2023

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
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants