musicblocks icon indicating copy to clipboard operation
musicblocks copied to clipboard

let there be let

Open walterbender opened this issue 4 years ago • 26 comments

Now that ECMA is ubiquitous, there is no longer any reason to not use let instead of var where appropriate. It is a fairly major change -- we need to be careful regarding scoping. But nonetheless, it is time.

  • [ ] js/
  • [x] js/widgets/
  • [x] js/blocks/ <-- mostly already in place
  • [x] js/utils
  • [x] planet/

walterbender avatar Apr 02 '20 20:04 walterbender

I will be working on this issue.

Ishakikani9117 avatar Apr 02 '20 20:04 Ishakikani9117

I will be working on this issue.

Already started, this is actually in reply to the commit I made. Maybe we can talk about how to share the work, so we don't spend time working on the same things?

Tobenna-KA avatar Apr 02 '20 20:04 Tobenna-KA

@Tobenna-KA ohh. same here. I also already started. Which part are you doing right now?

Ishakikani9117 avatar Apr 02 '20 20:04 Ishakikani9117

@Ishakikani9117 I was heading into js/

Tobenna-KA avatar Apr 02 '20 20:04 Tobenna-KA

@Tobenna-KA great , I was doing blocks and i will be heading to widgets and utils.

Ishakikani9117 avatar Apr 02 '20 20:04 Ishakikani9117

Alright, sounds good, I will do the work on what is outside those 3 folders within /js

Tobenna-KA avatar Apr 02 '20 20:04 Tobenna-KA

@walterbender Should we also try moving towards Arrow functions as there are numerous instances where this binding has been an issue, in the entire code. We can get rid of them using Arrow functions.

aviral243 avatar Apr 03 '20 06:04 aviral243

yes... we can use arrow functions in a lot of places.

walterbender avatar Apr 03 '20 14:04 walterbender

@walterbender Will create a new issue and start working on it ASAP.

aviral243 avatar Apr 04 '20 00:04 aviral243

@Ishakikani9117 @Tobenna-KA no offense. But, please be careful with the variable scopes while replacing var with let. I'm sure you know that let, unlike var, has a block only scope.

Here is an example. #2214 is caused by such a mistake. See the following excerpt in FlowBlocks.js: Screenshot from 2020-04-09 08-03-46

meganindya avatar Apr 09 '20 02:04 meganindya

@Ishakikani9117 @Tobenna-KA no offense. But, please be careful with the variable scopes while replacing var with let. I'm sure you know that let, unlike var, has a block only scope.

Here is an example. #2214 is caused by such a mistake. See the following excerpt in FlowBlocks.js: Screenshot from 2020-04-09 08-03-46

I'm pretty sure we both know this, and at least with the changes I have made in cases like this, you'd see me moving variable declarations out of the scope to a more global position to allow the use of let rather than var, but if necessary leave the var declaration.

But thank you for your concern.

Tobenna-KA avatar Apr 09 '20 04:04 Tobenna-KA

@Tobenna-KA Are you working on the remaining files inside js/ folder? Once they're done I can resume working on #2201

aviral243 avatar Apr 09 '20 06:04 aviral243

@Tobenna-KA great , I was doing blocks and i will be heading to widgets and utils.

@Ishakikani9117 I was recently working on quite a few issues with the phrase maker (widgets/pitchtimematrix.js). There's one I'm currently working on and it might require changes at several places. So, can you please leave out that file for the time being? After the issue is sorted, I can help with the replacements by let and arrow functions for that file, since I'm somewhat versed with its contents.

meganindya avatar Apr 09 '20 06:04 meganindya

@meganindya Ok. I will leave that file.

Ishakikani9117 avatar Apr 09 '20 07:04 Ishakikani9117

@Tobenna-KA Are you working on the remaining files inside js/ folder? Once they're done I can resume working on #2201

Yes I am

Tobenna-KA avatar Apr 09 '20 23:04 Tobenna-KA

@meganindya @Ishakikani9117 @Tobanna-KA Is anyone doing changes to planet. I want to work on this if anyone has not yet started on this.

b18050 avatar Apr 18 '20 11:04 b18050

@b18050 I want to do the planet folder.

Ishakikani9117 avatar Apr 18 '20 13:04 Ishakikani9117

@walterbender I've reviewed pull requests for changes in planet/ (#2231) and js/utils (#2225, #2226). Please take a look.

meganindya avatar May 09 '20 09:05 meganindya

The bulk of the work remaining is with the widgets.

walterbender avatar Nov 28 '20 23:11 walterbender

While I marked this as a "Good first issue", please bear in mind: (1) global replace will undoubtedly break the code so please don't; (2) please test after making your changes.

walterbender avatar Nov 28 '20 23:11 walterbender

While I marked this as a "Good first issue", please bear in mind: (1) global replace will undoubtedly break the code so please don't; (2) please test after making your changes.

@walterbender remaining files of js/widgets have been cleaned up, and both the guidelines were kept in mind while doing so.

ricknjacky avatar Nov 29 '20 11:11 ricknjacky

can i work on this issue ?

blank-27 avatar Dec 15 '20 08:12 blank-27

The js/ directory is the priority. Only 5 effective occurrences remain, probably corner cases. You may try #2629.

meganindya avatar Dec 15 '20 11:12 meganindya

ohk thanks

blank-27 avatar Dec 15 '20 12:12 blank-27

Please allow me to further clean it up in the remaining files.

Ayush-Khandelwal-007 avatar Mar 30 '21 17:03 Ayush-Khandelwal-007

@walterbender @meganindya is there any work still pending?

S-kus avatar Jan 02 '22 09:01 S-kus