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: Moving to arrow functions #2201

Closed
wants to merge 5 commits into from

Conversation

aviral243
Copy link
Member

  • Fixes Use arrow functions instead instead of normal ones #2198

  • All functions that are called somewhere have been converted. However, functions used as a constructor e.g HelpWidget() should remain as it is because arrow functions cannot be used as constructors.

  • I've also removed this binding wherever possible.

  • Regarding testing: I've tried to test each file after conversion but try reporting any errors that might show up.

@fakela
Copy link
Contributor

fakela commented Apr 4, 2020

Good work `going on ... I will test the various files changed

@aviral243
Copy link
Member Author

@fakela Thanks. That would be really helpful.

@aviral243
Copy link
Member Author

Possible errors that might creep in include:

  1. Lexical scoping of arrow functions i.e they are not hoisted so if any function is used before the declaration, an error would occur.

  2. Use of arrow functions for an Object method.
    e.g.
    var obj = { x: 9, decrease: () => { this.x--; } }
    When calling obj.decrease, x will not decrease as this is not bound to anything.

  3. Some functions have an argument passed named that, so care needs to taken not to replace them in haste.

  4. Dynamic callbacks functions may also lead to some errors when using arrow functions, though I'm not very sure about it. I've used it in a couple of places till now and it seems to work fine.
    e.g.
    document.addEventListener("mousemove", (e) => { window.hasMouse = true; });

This can be used as a reference to track down possible errors, both by me and people reviewing.

@walterbender
Copy link
Member

Maybe hold off working on js/blocks/* until #2196 lands. Otherwise, I imagine conflicts galore.

@aviral243
Copy link
Member Author

Maybe hold off working on js/blocks/* until #2196 lands. Otherwise, I imagine conflicts galore.

I was thinking the same thing earlier today. Will hold it off.

@aviral243
Copy link
Member Author

@walterbender Removed conflicts for now. ( Over 200 of them were present ). I think the work should be straightforward from here, touching only those files which keep getting merged as part of #2192

On that note, inside js/blocks/, since class components of different blocks need not be converted , the work is pretty minimal.

@aviral243
Copy link
Member Author

I'm testing blocks mainly using the uber project used for testing Block API.

uber.zip

@walterbender
Copy link
Member

Sorry about the conflicts. I accidentally merged something. Thanks for sorting it out.

@Ishakikani9117
Copy link
Contributor

@walterbender Removed conflicts for now. ( Over 200 of them were present ). I think the work should be straightforward from here, touching only those files which keep getting merged as part of #2192

On that note, inside js/blocks/, since class components of different blocks need not be converted , the work is pretty minimal.

@aviral243 Please do the same thing for js/widgets and js/utils as you have done for blocks ,so none of us would have merge conflicts.

@aviral243
Copy link
Member Author

Sorry about the conflicts. I accidentally merged something. Thanks for sorting it out.

@walterbender Not an issue there. They were mostly easy to resolve.

touching only those files which keep getting merged as part of #2192

Now following this strictly.

Action, Boolean, Boxes, Drum, Ensemble, Extras, Flow, Graphics, Heap, Intervals
Media, Meter, Number, Ornament, Pen, Pitch, RhythmPalette, Rhythm, Sensors, Tone, Volume, Widget
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use arrow functions instead instead of normal ones
4 participants