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

[WIP] Create Typescript declarations for Mixxx Javascript APIs #2636

Closed
wants to merge 10 commits into from

Conversation

Swiftb0y
Copy link
Member

@Swiftb0y Swiftb0y commented Apr 8, 2020

Discussion: https://mixxx.zulipchat.com/#narrow/stream/109171-development/topic/TypeScript.2Ffuture.20mapping.20system

Most code-completion tools can read typescript declaration files and JsDoc annotations.
This would be a great DX improvement for new people trying to create a new controller mapping.

I'm planning to add the JsDoc annotations soon, as well as creating declarations for midi-components and common-controller-scripts. I just wanted to create a PR now so someone can verify I didn't miss anything so far.

Add typescript declaration file for built-in Javascript APIs to
improve DX when creating mappings.
@Be-ing
Copy link
Contributor

Be-ing commented Apr 8, 2020

To clarify, IDEs can use this with plain JS? I don't want to require TypeScript, and probably don't want any transpiled scripts.

@Swiftb0y
Copy link
Member Author

Swiftb0y commented Apr 8, 2020

Generally, TS was designed to be compatible with JS. So even TS libraries can be used with plain JS because the TS libraries get distributed as JS and matching declaration files. So every IDE that understands .d.ts files, should also use them when editing regular JS files.

So, yes. IDE's should be using this with plain JS since typescript was designed with that in mind.

@Holzhaus
Copy link
Member

Holzhaus commented Apr 8, 2020

Can we use this file for linting with pre-commit? Can eslint read this file?

@Swiftb0y
Copy link
Member Author

Swiftb0y commented Apr 8, 2020

Not without additional tools unfortunately, theoretically, one could run tsc --allowJS on the file and that would typecheck it iirc but eslint itself can't do it out of the box. Running tsc on the file would require a full blown typescript installation which we want to avoid. The purpose of the file is primarily to improve DX (though its also possible to check with tsc if that is already installed).

@Holzhaus
Copy link
Member

Holzhaus commented Apr 8, 2020

pre-commit installs dependencies automatically. I think it's available on npm, so it should work

@Swiftb0y
Copy link
Member Author

Swiftb0y commented Apr 9, 2020

Well, that said, the just because the tsc would pass, it doesn't guarantee that the APIs are used correctly. eg some functions expect integers but JS/TS only has a number type. It would also still be possible to have typos in the group or key strings... So it would only still catch a small subset of potential errors.
@Be-ing what do you think?

@Holzhaus
Copy link
Member

Holzhaus commented Apr 9, 2020

Needs a bit more work, but: Swiftb0y#2

@Be-ing
Copy link
Contributor

Be-ing commented Apr 9, 2020

If we're telling mapping developers to use pre-commit for eslint anyway and it's no more trouble to use this for linting, I don't see why not.

@Swiftb0y
Copy link
Member Author

Using tsc for linting would also require a definition for all the other types exposed by QJSengine. This would in turn require another handwritten typescript definition file just for that. I think the amount of effort required to make it work is not worth the linting capabilities... So I'm against introducing tsc as a pre-commit hook!

@Holzhaus
Copy link
Member

So it isn't possible to tell tsc to only check defined types and ignore everything else?

@Swiftb0y
Copy link
Member Author

afaik tsc will throw errors when it encounters symbols which are not defined (even when not running in strict mode). I think you can still use --lib ES7 and it will recognize all symbols in ES7, but none that are Interpreter specific.

@Holzhaus
Copy link
Member

Are there any interpreter specific types except the stuff in custom-controller-scripts.js?

@Swiftb0y
Copy link
Member Author

Swiftb0y commented Apr 10, 2020

Not right now afaik, its still a case where tsc is enforcing arbitrary boundaries... I'll still try to make it work for testing purposes, but I am unsure about any consequences overly strict pre-commit rules (such as relying on tsc not complaining) have on the DX (the thing this PR is actually trying to improve).

@Swiftb0y
Copy link
Member Author

So I got linting with tsc to work... the next step forward would be to write declaration files for the entire API (so also other scripts such as common-controller-scripts.js). It wouldn't make sense to write the declaration files by hand in this case, but to rather use JSDoc comments which can be translated to ambient declarations automatically by tsc. So should I start writing JSDoc comments for common-controller-scripts.js @Be-ing?

@Be-ing
Copy link
Contributor

Be-ing commented Apr 10, 2020

should I start writing JSDoc comments for common-controller-scripts.js @Be-ing?

Sure

@ferranpujolcamins
Copy link
Contributor

Don't forget to document on the wiki how to configure some text editor to take advantage of this!

@Swiftb0y
Copy link
Member Author

Of course. Intellisense/vscode use it automatically. What other editors should I test?

@ferranpujolcamins
Copy link
Contributor

So imagine I have my script in a dedicated repo, how do I use this?

@Swiftb0y
Copy link
Member Author

Depends on what you would like to use it for... If its just supposed to be used for code completion, there has to be a copy of the file present somewhere in your repo as well. If used for linting, there has to be the specific folder structure present and tsc needs to be pointed to the typings folder via the typeRoots option.

@randombyte-developer
Copy link

I have also made a d.ts file for Mixxx. I only implemented the things I really used in the mapping.

What stood out to me when comparing the definitions was that I have a function log here.
And my mapping's get/setValue() use numbers or booleans: here, but I am actually not sure about that right now if that's correct.
I also spotted some differences of how we express the types, I don't know which one is better. Actually I have only touched Typescript once in my life yet, and it was for this controller mapping.

@Swiftb0y
Copy link
Member Author

What stood out to me when comparing the definitions was that I have a function log here.

Yes. I actually forgot that there was an engine.log and print is just a wrapper around it in common-controller-scripts.js

And my mapping's get/setValue() use numbers or booleans: here, but I am actually not sure about that right now if that's correct.

getValue/setValue essentially just set values on Mixxx ControlObjects (which always have a store the value as a double). Every number can be implicitly cast too a boolean, but since the COs only really contain doubles, the JS Api only really returns numbers. The point being, you could probably remove the boolean in your definition and it wouldn't require any changes in your actual code.

I also spotted some differences of how we express the types, I don't know which one is better.

Autocompleters are usually very lenient about what is what, so it doesn't matter to them whether the functions are contained in a namespace or an interface but from the pov of tsc, it would matter. If an interface is used, tsc will essentially think engine is just the shape of an object. So it wouldn't complain when you try to implement that interface or extend it (because the engine namespace is just a namespace not a class or an object).

You also use type[] for declaring arrays, but I favor Array<type> because its more expressive (IMO) and it looks like (and behaves similar to) C++ templates.

I used typescript for writing a project I created for a paper for school. Took me about 4 months but it unfortunately never really saw the light of day. Still, I used it enough to know that I don't really like using it but its ambient declaration are good for things like improving developer declaration and catching typeerrors during compile time.

@github-actions
Copy link

This PR is marked as stale because it has been open 90 days with no activity.

@github-actions github-actions bot added the stale Stale issues that haven't been updated for a long time. label Oct 19, 2020
@Swiftb0y Swiftb0y marked this pull request as draft October 19, 2020 08:30
@Be-ing Be-ing closed this Nov 25, 2020
@Be-ing Be-ing deleted the branch mixxxdj:main November 25, 2020 15:51
@Be-ing Be-ing reopened this Nov 25, 2020
@Be-ing Be-ing changed the base branch from master to main November 25, 2020 16:04
@JoergAtGithub
Copy link
Member

This PR is superseded by #4759 which is merged now

@Swiftb0y
Copy link
Member Author

Yep, ty

@Swiftb0y Swiftb0y closed this May 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code quality controller mappings stale Stale issues that haven't been updated for a long time.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants