Use of arrow functions for prototype definition

[stalgiag]

Since the ES6 update has been merged already, I am moving a discussion out of #3874 and into an issue so that it is easier to track.

The issue as outlined by @limzykenneth in the pull request discussion:

Great work @hsab!

I have a particular question on the use of arrow functions to define prototype members. From what I can find it seems to affect the value of this in the member function (arrow function's this is defined when the function is evaluated, not when the object is created as such it doesn't point to the object instance created). For a discussion about it here and here.

I can't identify with a quick look whether it is a problem here or not but if the use of arrow function on prototype member isn't really doing anything other than saving bytes it seems a bit superflous to me.

@hsab replied:

@limzykenneth Thanks!

This is certainly a valid and concerning point. I will have a thorough look tomorrow.
Although I was aware of this behavior, I can't recall running into any issues with this.
Also, just did a quick look and it seems like exactly what you mention is happening in main.js. But these functions (_start, _draw, _setup) are critical (right?) and yet this seems to point to object instance during execution. This is quite a curious case. Definitely needs more investigation. If you have an idea of why, I would very much appreciate it.

followed by @limzykenneth :

@hsab From my understanding, basically in the main.js case you pointed out, at the time the arrow function is evaluated, the value of this is the same as the value of this._start's this since they are defined in the same scope and so share the same value for this. However, when attaching to the prototype, the value of this should be defined at the point of object creation (when new Something() is called) but with arrow functions, the value of this in the function will be defined where the function is defined.

So in simpler terms, the issue will be centered around the usage of arrow functions on prototype members, the rest should not encounter problems and in my opinion ok to use arrow functions.

Note that this doesn't seem urgent so feel free to continue discussion and response at your own convenience. I just wanted a better place to track the topic. Thanks!

[meiamsome]

The conversion is correct in maintaining behaviour - it has converted functions of two forms:

1. those that do not use this (And thus it is irrelevant)
2. those that used something of the form:

function(/* ... */) {
  // ...
}.bind(this)

In the old source code, _start, _draw and _setup all have a .bind(this) statement on them. Whether they should have had that or not is a different debate to have, but that transformation is preserving behaviour correctly.

[limzykenneth]

Yes, for the cases that was using .bind(this) arrow functions definitely is the way to go. From my quick glance, there isn't much instances of what I think may cause problems which is using arrow functions for prototype members, ie. p5.prototype.myMethod = () => { ... }, but there are at least a few. For me I think even if the prototype member itself isn't using this in the function body, I would like to avoid replacing regular anonymous functions with arrow functions, both for future possibilities (even if remote) of said function needing to use this, and also arrow functions in this case isn't giving much value beyond saving bytes. (Unless there are some other purpose that I didn't know of?)

Any other uses of arrow functions should not be causing much problem I think.

[limzykenneth]

Actually does our current setup for babel converts arrow functions into regular functions? @outofambit

[stalgiag]

My personal opinion is that all prototype members should use the old style p5.prototype.myMethod = function( ) { }. This helps clarify the status of the context and it also would add some consistency of style. Currently there are locations where both approaches are used within the same file. This is fine from a functionality standpoint but for new contributors this might appear to have hidden significance.

[outofambit]

Actually does our current setup for babel converts arrow functions into regular functions?

@limzykenneth yes, it does! it most likely converts them into the bind version

@stalgiag @limzykenneth how wold you feel about something like this for all methods? i don't think we use the unbound this functionality anywhere so we could standardize on something like this.

p5.prototype.myMethod = () => {}

the nice thing about the above form is that we don't bind some functions and not others. i guess part of why i'm suggesting this is that i find the function() {}.bind(this) form to be harder to read because i often don't look all the way to the end of the function to see the bind part, then when if i see the function call this somewhere, i have to go to the end of the function to see.

i don't feel strongly about this, just wanted to throw this option out there. Definitely agree we want to be consistent no matter what it is. :)

[hsab]

I'm slightly tilted towards @stalgiag and @limzykenneth proposed use of function() {} for prototypes.

@outofambit was wondering how you see p5.prototype.myMethod = () => {} play out when this is in fact used, given that rebinding is not permitted with arrow functions. This came up with the example @stalgiag posted, comparing L165 with L183

[limzykenneth]

@outofambit My opinion is inline with @stalgiag and @hsab in that for prototype members they should all use function() {} instead of arrow functions, even if they don't use this in the function body.

For functions that needs .bind(this) they definitely should be converted to arrow functions as that is the reason arrow functions are created in the first place. I don't we there are any instances where .bind(this) is used on a prototype member, and I would find that rather weird.

[outofambit]

@hsab oo thats a good example, thanks for sharing that! that could be tricky to refactor.

@limzykenneth that works for me! sounds like we have a consensus :)

[lmccart]

@limzykenneth I wasn't able to find the instances of the p5.prototype.myMethod = () => {} pattern that you mentioned seeing. can you point me toward one of them so I can figure out what I'm doing wrong in my searching?

[limzykenneth]

I remember first seeing it in the PR that made the transition to ES6. I’m not able to do too much digging at the moment as my computer is off for repairs but I’ll see if I can find any when I have more access.

[limzykenneth]

For a quick one, ‘src/color/color_conversion.js > p5.ColorConversion._hsbaToHSLA’.

[limzykenneth]

@lmccart Got my computer back and I have some free time to work on some stuff, I'll add this to my list.