common-controller-scripts.js: Convert "script" to freezed object, and remove global "controller", "deck", "control", "button" definitions#11625
Conversation
180edac to
210248a
Compare
…yntax Silenced eslint warnings or unused vars
210248a to
b36c378
Compare
daschuer
left a comment
There was a problem hiding this comment.
LGTM, However I am unable to test this.
Can we risk to merge it and see what happens during the remaining 5 Weeks beta?
|
I think it's unecessary risk for 2.4 now. |
|
This PR is marked as stale because it has been open 90 days with no activity. |
|
Oh sorry this one slipped through. Can you fix the conflicts and than we can merge? |
Swiftb0y
left a comment
There was a problem hiding this comment.
not sure if thats the culprit of the CI failure though.
3c327c1 to
00a5b43
Compare
|
CI fails. I like to move this either to 2.5.0 or 2.6-alpha milestone. Does this have an impact on mapping interface regarding documentation? |
|
No impact on documentation. I currently don't know how to fix this. |
|
Let's wait for the end of the CI but I believe ac4ab33 solves this CI error. At least it solves it locally here. I think the error comes from that I don't know why reverting to Edit: yup, that fixes the tests. You can cherry pick |
|
Great. Thanks for investigating. |
|
Unfortunately the |
|
Now everything is Pass! |
|
great. the only remaining comments are the ones from above suggesting the use of default parameters. Have you looked into that? EDIT: I just double-checked myself. It does support default parameters. So it be nice to see them used in the spots highlighted. |
|
Done! |
| } | ||
| } | ||
| // Add instance bpm of bpmClass to the Global JavaScript object | ||
| this.bpm = new bpmClass(); |
There was a problem hiding this comment.
Why didn't you keep the IIFE in my fix?
There was a problem hiding this comment.
iiuc because it broke editor support for Joerg.
There was a problem hiding this comment.
What about using JSDoc's @global?
| this.bpm = new bpmClass(); | |
| (function(global){ | |
| class bpmClass { … } | |
| /** @global */ | |
| const bpm = new bpmClass(); | |
| … | |
| global.bpm = bpm; | |
| })(this); |
I used it here. @JoergAtGithub did it do the trick for you?
There was a problem hiding this comment.
I just tried it again, with the proposed code the symbol bpm becomes invisible outside of the function statement. Neither code completion, pop-up help nor type checking work anymore. While with the code of this PR everything works.
this.bpm = new bpmClass(); is not a new code pattern, it's already used in the hid-parser of 2.4 and seems to work for everybody.
There was a problem hiding this comment.
Weird. But ok then.
Edit: but you don't have any problem with Lodash? 🤔
| const colorCodeFromObject = function(color) { | ||
| return ((color.red & 0xFF) << 16 | (color.green & 0xFF) << 8 | (color.blue & 0xFF)); | ||
| }; |
There was a problem hiding this comment.
did you double check the const here didn't break anything?
| // @ts-ignore Same identifier for class and instance needed for backward compatibility | ||
| class bpmClass { |
There was a problem hiding this comment.
| // @ts-ignore Same identifier for class and instance needed for backward compatibility | |
| class bpmClass { | |
| class bpmClass { |
I also just noticed that we now have [ChannelN],tempo_tap which essentially deprecates script.bpm, right? cc @ronso0
|
This PR is marked as stale because it has been open 90 days with no activity. |
|
What is the state here, is this still a 2.5.0 candidate? |
|
@JoergAtGithub This one has developed conflicts |
console.loginstead of deprecatedprintfunction