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

Declarations for engine and controller scripting API #4759

Merged
merged 36 commits into from
May 13, 2023

Conversation

JoergAtGithub
Copy link
Member

@JoergAtGithub JoergAtGithub commented May 12, 2022

These declarations allow modern code editors and IDEs, to show online syntax help and allows type checking (as far as JavaScript supports types).

To enable this, the mapping developer has to create a file jsconfig.json in the directory with the JavaScript mapping to edit. This file is editor independend an should work with all editors, which support the Language Server Protocol (LSP), like VS Code, Atom, Eclipse, Kate, Sublime, etc.

The jsconfig.json must have the a content like below, that allows the editor to find all include files and tells him to use ES6 syntax:

{
  "compilerOptions": {
    "target": "es6",
    "checkJs": true,
    "lib": ["ES6"]
  },
  "include": ["<mappingpath>/Traktor-Kontrol-Z2-hid-scripts.js",
    "<mixxxpath>/res/controllers/common-hid-packet-parser.js",
    "<mixxxpath>/res/controllers/common-controller-scripts.js",
    "<mixxxpath>/res/controllers/hid-controller-api.d.ts",
    "<mixxxpath>/res/controllers/engine-api.d.ts",
    "<mixxxpath>/res/controllers/console-api.d.ts"]
}

grafik

grafik

@JoergAtGithub JoergAtGithub marked this pull request as draft May 12, 2022 22:09
@Swiftb0y Swiftb0y self-assigned this May 12, 2022
@daschuer
Copy link
Member

daschuer commented Jun 8, 2022

Sounds reasonable. Does this put any mandatory work to the mapping developer, that makes work harder as a noob contributor?

@JoergAtGithub
Copy link
Member Author

Nothing mandatory, if you don't create the jsconfig.json file next to your mapping file, everything is as before.

@Swiftb0y
Copy link
Member

Swiftb0y commented Jun 8, 2022

Please ping me when this PR is ready for review. I'll take a look then.

@JoergAtGithub JoergAtGithub marked this pull request as ready for review June 9, 2022 20:48
@JoergAtGithub
Copy link
Member Author

Please ping me when this PR is ready for review. I'll take a look then.

@Swiftb0y This is now ready for review!

Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

Thank you very much for working on this DX improvement.
Please also link to some parts of our more elaborate docs on the github wiki.
Also, have you considered using plain JS files containing just JSDoc comments?
Also please add a comment to the *JSProxy-classes to point them to these definition files so they get updated when the proxy gets updated.

res/controllers/console-api.d.ts Outdated Show resolved Hide resolved
res/controllers/console-api.d.ts Outdated Show resolved Hide resolved
res/controllers/console-api.d.ts Outdated Show resolved Hide resolved
res/controllers/console-api.d.ts Outdated Show resolved Hide resolved
res/controllers/console-api.d.ts Outdated Show resolved Hide resolved
res/controllers/hid-controller-api.d.ts Outdated Show resolved Hide resolved
res/controllers/hid-controller-api.d.ts Outdated Show resolved Hide resolved
res/controllers/hid-controller-api.d.ts Outdated Show resolved Hide resolved
res/controllers/hid-controller-api.d.ts Show resolved Hide resolved
res/controllers/midi-controller-api.d.ts Outdated Show resolved Hide resolved
@Swiftb0y
Copy link
Member

@JoergAtGithub please also take a look at my previous but abandoned attempt at this #2636

@JoergAtGithub
Copy link
Member Author

@JoergAtGithub please also take a look at my previous but abandoned attempt at this #2636

Wasn't aware of this PR. I'm not at home this week, and will address your review comments the week after.

@Swiftb0y
Copy link
Member

No problem. I just wanted to supply some context for my review comments.

@JoergAtGithub JoergAtGithub marked this pull request as ready for review May 1, 2023 21:43
Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

thank you, just minor wording suggestions. I've tried to test this but I struggled getting it to work in my editor (vscodium). I'll get back to you once I figured out whats wrong.

.pre-commit-config.yaml Outdated Show resolved Hide resolved
res/controllers/color-mapper-api.d.ts Outdated Show resolved Hide resolved
res/controllers/engine-api.d.ts Outdated Show resolved Hide resolved
res/controllers/engine-api.d.ts Outdated Show resolved Hide resolved
res/controllers/engine-api.d.ts Outdated Show resolved Hide resolved
res/controllers/engine-api.d.ts Outdated Show resolved Hide resolved
res/controllers/engine-api.d.ts Outdated Show resolved Hide resolved
res/controllers/engine-api.d.ts Outdated Show resolved Hide resolved
res/controllers/hid-controller-api.d.ts Outdated Show resolved Hide resolved
res/controllers/hid-controller-api.d.ts Outdated Show resolved Hide resolved
@Swiftb0y
Copy link
Member

Swiftb0y commented May 2, 2023

After getting the jsconfig to work, I'm really annoyed by the fact that a developer needs to edit the jsconfig to reference their mapping in order for everything to work. After a bit of research and trial and error, I don't think there is anything we can do against that, right?

We could get that working by making each mapping their own project (which would make sense) but that would require putting each mapping to its own folder. That's something I would like to do anyways in the future, but is out-of-scope for this PR.

I think we should include a jsconfig as well. Putting this into res/controllers/ should work:

{
    "compilerOptions": {
        "target": "es6",
        "checkJs": true,
        "lib": ["ES6"]
    },
    "include": [
        "*.d.ts",
        "common-hid-packet-parser.js",
        "common-controller-scripts.js",

        // add the filename of your own mapping here:
        ""

        // make sure to exclude any changes in this file from your submission
    ]
}

@JoergAtGithub
Copy link
Member Author

I would not like to add a jsconfig.json in the scope of this PR.

@Swiftb0y
Copy link
Member

I would not like to add a jsconfig.json in the scope of this PR.

Thats fair. Though it certainly makes it more difficult to test.

Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

thank you, last couple complaints I found.

res/controllers/hid-controller-api.d.ts Outdated Show resolved Hide resolved
res/controllers/midi-controller-api.d.ts Outdated Show resolved Hide resolved
res/controllers/color-mapper-api.d.ts Show resolved Hide resolved
res/controllers/engine-api.d.ts Outdated Show resolved Hide resolved
Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

Indentation looks good to me. LGTM otherwise. Thank you for your patience.

res/controllers/engine-api.d.ts Outdated Show resolved Hide resolved
res/controllers/engine-api.d.ts Outdated Show resolved Hide resolved
@JoergAtGithub
Copy link
Member Author

Done

Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

LGTM, thank you.

@Swiftb0y Swiftb0y merged commit 79acc08 into mixxxdj:2.4 May 13, 2023
@JoergAtGithub
Copy link
Member Author

Time to celebrate twice - the PR is exactly 1 year old today. 🎂

@JoergAtGithub JoergAtGithub deleted the controllerApiDeclarations branch May 13, 2023 18:00
@Swiftb0y
Copy link
Member

Sorry, this took this long... Highly appreciate the effort though.

@JoergAtGithub
Copy link
Member Author

Thank you for the review!

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

Successfully merging this pull request may close these issues.

3 participants