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

Migrate to typescript #3

Merged
merged 8 commits into from
May 13, 2020
Merged

Conversation

jraoult
Copy link
Collaborator

@jraoult jraoult commented May 9, 2020

WIP: this is just a draft for now so that we can iron the details.

What's this PR do?

This is an effort to convert the code of the most advanced branch ( the one adding the support of Maschine MK3) to Typescript. I'm using the last stable version of TS (3.8 as of 9th May 2020). I think this should eventually become the master branch and should renamed (may be ni-controllers-lib?) and published to NPM.

This is mostly a compatible version with the previous one. The only major change is the replacement of default exports to named ones. I feel having the name of the class forced by the implementation helps reading and debugging (cf. https://humanwhocodes.com/blog/2019/01/stop-using-default-exports-javascript-module/). Happy to remove them if you disagree with this. I still need to add the JS transpilation to the NPM lifecycle so that it is usable without typescript by consumers (like your taskolio app).

I focused on on typescript transformation only to minimise the scope of the PR but I notice a couple of things that I'm happy to work on later:

I also added prettier and formatted the code base with it to get a consistent code style.

How should this be tested by the reviewer?

I put together a quick playground.ts script. It can be run with npm run playground. For now it just lit one pad to red and listen for pad events. This is just fr quick test purpose, I'll remove it once the PR is ready for merging.

What are the relevant tickets?

#2

@jraoult
Copy link
Collaborator Author

jraoult commented May 9, 2020

Here we go @asutherland let me know what you think. I'll start the work on the USB abstraction from this branch

@asutherland
Copy link
Owner

Everything looks great so far!

The only major change is the replacement of default exports to named ones.

Agreed on this and the rationale.

* I really like what you started to do with the events in `PacketizedPads`:

Yes, I'd be very happy to see the eventing cleaned up on this.

I also added prettier and formatted the code base with it to get a consistent code style.

Thanks! This is also a very nice improvement!

@jraoult jraoult marked this pull request as ready for review May 12, 2020 08:29
@jraoult
Copy link
Collaborator Author

jraoult commented May 12, 2020

@asutherland I think this is ready to go.

I changed the name of the package to ni-controllers-lib and set the version to 1.0.0-alpha.1. I tested publish on my user scope and it works. You can use this for your own test if you want:

npm install @jesuisjo/ni-controllers-lib

I'll keep this PR as just the one to transform to Typescript. I have a set of PRs coming after to fix the step wheel, enable WebUSB, improve the eventing mechanism, simplify the LCD api (so that users can just pass bitmaps for example) but I wanted to keep the this one's scope small.

So now the imports should be:

import { MaschineMk3 } from "ni-controllers-lib/dist/maschine_mk3";

or when using commonjs

const { MaschineMk3 } = require("ni-controllers-lib/dist/maschine_mk3");

Pay attention to the dist part of the path, there are ways to get rid of it when publishing to NPM but it is quite convoluted (cf. https://stackoverflow.com/questions/38935176/how-to-npm-publish-specific-folder-but-as-package-root) and I'm not convinced it is worth the pain.

Let mew know what you think about it. If it's all good, I feel we should make this branch the master and then publish on NPM as a v1.0.0. Might be worth eventually renaming the repo as well eventually but it's not urgent.

Copy link
Owner

@asutherland asutherland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wooooo! 🎉 🎉 🎉 🎉 🎉

@asutherland asutherland merged commit eeb7247 into asutherland:maschine-mk3 May 13, 2020
@asutherland
Copy link
Owner

asutherland commented May 13, 2020

This is merged in. I've also:

I'm quite happy to review pull requests for your changes and I think I should be able to get to them in a timely fashion and it's a good workflow, but please do feel free to self-merge things that you feel are trivial or if it seems like I've disappeared and will never review something.

@jraoult
Copy link
Collaborator Author

jraoult commented May 16, 2020

@asutherland apologies for the delay. I actually moved place this week so I was quite busy.

Brillant! I have the USB refactoring branch almost ready. I'll create a PR this week to keep the ball rolling while I'm deep into it. I'll probably ask your review for this one since it is a breaking change, just so that you are aware what you need to do for your app. Then the step wheel fix is ready as well, I probably won't need your review for this one.

@jraoult jraoult deleted the mk3_to_ts branch May 16, 2020 09:53
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

Successfully merging this pull request may close these issues.

None yet

2 participants