Widgets Var->let Cleanup

[ricknjacky]

var->let clean up pertaining to js/widgets/
part of fix for #2192

[ricknjacky]

There are still some var in status.js and it's better to keep them that way, I tested and changing them to let/const breaks the widget's functionality. For the rest part status.js has been cleaned up.

[meganindya]

What's the cleanup you performed? Because there are remaining traces of var. If we need the port, we need to do it entirely.

[meganindya]

#2634 will conflict with this. That one's a rather simple one. How about you review that one?

We actually better get rid of all instances of that.

[meganindya]

There are still some var in status.js and it's better to keep them that way, I tested and changing them to let/const breaks the widget's functionality. For the rest part status.js has been cleaned up.

I think the problem is with redeclaration. The variable you saw declared with var is probably used outside it's scope. var works, let won't. The solution is to ramp up the scope of that variable.

The code becomes harder to follow and this process is indeed a lot tedious to do and review, but it doesn't look like we have much of an option as of now. We better not keep coming back to the same files again and again.

I have done and reviewed a lot of the previous conversions. High possibility of practically untraceable errors.

[ricknjacky]

I think the problem is with redeclaration. The variable you saw declared with var is probably used outside it's scope. var works, let won't. The solution is to ramp up the scope of that variable.

True, but the revamp and getting rid of that and var you're rightly suggesting; is part of #2629

The code becomes harder to follow and this process is indeed a lot tedious to do and review, but it doesn't look like we have much of an option as of now. We better not keep coming back to the same files again and again.

I have done and reviewed a lot of the previous conversions. High possibility of practically untraceable errors.

I agree, but this PR was in regard to #2192 where priority was porting to var->let, such that it doesn't break the code.

I was hence planning on completing that first, then get on to basically get rid of the var and that respectively

[ricknjacky]

@walterbender I know the fix for #1144 however it'd better if this PR is merged with master and then, I start working on it. To avoid conflicts galore, that'd follow otherwise.
I fondly anticipate your feedback on this

[walterbender]

This could be fixed independently of the var / let issue

[ricknjacky]

Ok
I'll fix it though, but are there any issues with this PR? @walterbender

[ricknjacky]

I just checked now there are conflicts as you too have worked on same files as I already did.
What do you suggest me to do now?

[walterbender]

Sorry for the conflict. Can you rebase and just work on musickeyboard and phrasemaker? The rest are done.

[ricknjacky]

No issues, I will work on them now
The conflict is only with js/widgets/rhythmruler.js and js/widgets/status.js. I will rebase

[ricknjacky]

Please review @walterbender

[meganindya]

Could you give me some context about what you've done. I'll review this and get it sorted then

[ricknjacky]

Could you give me some context about what you've done. I'll review this and get it sorted then

I changed var to let in js/widgets files. I have tested each widget for the same. I didn't replace all of them, basically didn't get rid of all them, because doing so breaks the code.

[ricknjacky]

The code becomes harder to follow and this process is indeed a lot tedious to do and review, but it doesn't look like we have much of an option as of now. We better not keep coming back to the same files again and again.

True, but some files have 3k+ lines of code and the revamp that we are planning to do, I think should deal with them 1 file at a time. And this PR is ig, stepping stone for the future revamp

[meganindya]

That'll be done, but the ES6 conversion better be sorted here itself. We wouldn't want problems during the port. Also, given the fact that it'll be in TypeScript would be an even bigger challenge.

[ricknjacky]

Great

So, to not clutter this PR anymore
Should I create a new PR wherein I port to arrow functions?

[meganindya]

Could you give me some context about what you've done. I'll review this and get it sorted then

I changed var to let in js/widgets files. I have tested each widget for the same. I didn't replace all of them, basically didn't get rid of all them, because doing so breaks the code.

Can you point out any instances when it crashes? I'll check them. I'm sort of adamant we need to get rid of var entirely. Why would the JS developers add such peculiar behavior at all? 🤯

[ricknjacky]

Can you point out any instances when it crashes? I'll check them. I'm sort of adamant we need to get rid of var entirely. Why would the JS developers add such peculiar behavior at all? 🤯

Well there were a couple of them .
I can recall one such instance recently when I was working with musickeyboard, where var w instance on being ported to let w broke the widget i couldn't open the widget. I'll attach a gif for the same if you want

[meganindya]

I think it's already getting out of hand with so many concurrent PRs. What I think would be cool is to close this PR and create new ones, one file at a time, (a) replacing all var with let/const, (b) replacing callbacks with arrow functions and (c) getting rid of all instances of that since arrow functions sort that issue.

"one at a time" - seriously mean it ;p

[meganindya]

You already have the commits. You can cherry pick and modify them rather easily.

[ricknjacky]

I think it's already getting out of hand with so many concurrent PRs. What I think would be cool is to close this PR and create new ones, one file at a time, (a) replacing all var with let/const, (b) replacing callbacks with arrow functions and (c) getting rid of all instances of that since arrow functions sort that issue.
"one at a time" - seriously mean it ;p

Ok ;-; Will do as you say

[meganindya]

solving all three issues with each file

[meganindya]

Don't bother about the issue tickets. These are little but are widespread and tedious. We'll close the issues manually, once done.

[ricknjacky]

You already have the commits. You can cherry pick and modify them rather easily.

Yes.
Are we merging this PR?
If not, I'll create a new PR for each file and will do var->let all over again.

[meganindya]

Probably better if you create new ones.

[ricknjacky]

Got it, will do.

[meganindya]

How many remain?

BTW, I'm converting this to draft. Close this when all are done.

I'm unsubscribing from this ticket.

[walterbender]

I am closing this PR. Can you please open two new ones: one for phrasemaker.js and one for musickeyboard.js?

[ricknjacky]

I am closing this PR. Can you please open two new ones: one for phrasemaker.js and one for musickeyboard.js?

Sure, Please check #2677 for phrasemaker.