1151474 - Remove use of expression closures in im/

Status: RESOLVED FIXED

(Instantbird Graveyard :: Other, defect)

Description

[Tooru Fujisawa [:arai]]

Similar to bug 1123124.

Need to replace non-standard expression closure `function() expr` with one of function declaration, function expression, and arrow function, before fixing bug 1083458.

I have draft patch for this.

Comment 1

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Remove use of expression closure from im/components/.

Thank you for reviewing the patch in bug 1151475 :)
Can I ask review for this bug as well? (4 patches)

same converting rules:
  * function declaration
    add `return` and braces
  * standalone function expression contans and receives `this` (Array.filter, bind, etc)
    convert to arrow function
  * standalone function expression contans no `this`
    convert to arrow function
  * method-like property contains `this`
    add `return` and braces
  * method-like property contains no `this` with short body
    convert to arrow function
  * method-like property contains no `this` with long body
    add `return` and braces
  * method-like property with named function expression
    add `return` and braces
  * getter property
    add `return` and braces
  * setter property
    add braces

Tested in same try run, and mozmill on local run (19 failures with/without patches).
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=3ea958f2cad9

Comment 2

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Remove use of expression closure from im/contents/.

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: Part 3: Remove use of expression closure from im/modules/.

Comment 4

[Tooru Fujisawa [:arai]]

Attachment: Part 4: Remove use of expression closure from im/themes/.

Comment 5

[Patrick Cloke [:clokep]]

(In reply to Tooru Fujisawa [:arai] from comment #1)
> same converting rules:
>   * standalone function expression contans and receives `this`
> (Array.filter, bind, etc)
>     convert to arrow function
>   * standalone function expression contans no `this`
>     convert to arrow function
>   * method-like property contains no `this` with short body
>     convert to arrow function

I don't understand what these mean, can you please provide example.

I find it a real shame we need to make any of these changes, function expression closures are a super useful feature.

> Tested in same try run, and mozmill on local run (19 failures with/without
> patches).
> https://treeherder.mozilla.org/#/jobs?repo=try-comm-
> central&revision=3ea958f2cad9

Try builds aren't useful for im, unfortunately.

Comment 6

[Tooru Fujisawa [:arai]]

(In reply to Patrick Cloke [:clokep] from comment #5)
> (In reply to Tooru Fujisawa [:arai] from comment #1)
> > same converting rules:
> >   * standalone function expression contans and receives `this`
> > (Array.filter, bind, etc)
> >     convert to arrow function
> >   * standalone function expression contans no `this`
> >     convert to arrow function
> >   * method-like property contains no `this` with short body
> >     convert to arrow function
> 
> I don't understand what these mean, can you please provide example.
> 
> I find it a real shame we need to make any of these changes, function
> expression closures are a super useful feature.

following case for each:

im/content/conversation.xml
-  .forEach(function(a) this.addBuddy(a.buddy, true), this);
+  .forEach(a => this.addBuddy(a.buddy, true));

im/content/blist.js
-  if (contact.getTags().some(function(t) t.id == aTag.id))
+  if (contact.getTags().some(t => t.id == aTag.id))

im/content/debug/fake/fake.js
-  normalize: function(aStr) aStr
+  normalize: aStr => aStr

> > Tested in same try run, and mozmill on local run (19 failures with/without
> > patches).
> > https://treeherder.mozilla.org/#/jobs?repo=try-comm-
> > central&revision=3ea958f2cad9
> 
> Try builds aren't useful for im, unfortunately.

oops, how can I test these changes?

Comment 7

[Florian Quèze [:florian]]

(In reply to Patrick Cloke [:clokep] from comment #5)

> I find it a real shame we need to make any of these changes, function
> expression closures are a super useful feature.

While I agree, I'm afraid this is a lost battle at this point.

The "get foo() this._foo," case that gets converted to "get foo() { return this._foo; }," is especially frustrating. On the up side, lots of cases where we can now use arrow functions are actually making the code look nicer.

Comment 8

[Florian Quèze [:florian]]

(In reply to Tooru Fujisawa [:arai] from comment #6)

> > > Tested in same try run, and mozmill on local run (19 failures with/without
> > > patches).
> > > https://treeherder.mozilla.org/#/jobs?repo=try-comm-
> > > central&revision=3ea958f2cad9
> > 
> > Try builds aren't useful for im, unfortunately.
> 
> oops, how can I test these changes?

There's currently no automated way to test im/, unfortunately. We produce nightly builds, so if something is really obviously broken, we'll usually have a bug filed within a day or two.

Comment 9

[Florian Quèze [:florian]]

Comment on attachment 8589771 [details] [diff] [review]
Part 2: Remove use of expression closure from im/contents/.

Review of attachment 8589771 [details] [diff] [review]:
-----------------------------------------------------------------

::: im/content/viewlog.js
@@ +194,5 @@
> +  get open() { return this._open; },
> +  get level() { return 0; },
> +  get _parent() { return null; },
> +  get children() { return this._children; },
> +  getProperties: function() { return ""; }

Is there a reason why this isn't:
  getProperties: () => ""
?

I'm not sure if you are doing this conversion by hand or with a script applying automatically the rules you listed in comment 1.

@@ +208,5 @@
> +  get id() { return this.log.title; },
> +  get open() { return false; },
> +  get level() { return this._level; },
> +  get children() { return []; },
> +  getProperties: function() { return ""; }

same here.

Comment 10

[Tooru Fujisawa [:arai]]

Thank you for reviewing!

(In reply to Florian Quèze [:florian] [:flo] from comment #9)
> Comment on attachment 8589771 [details] [diff] [review]
> Part 2: Remove use of expression closure from im/contents/.
> 
> Review of attachment 8589771 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: im/content/viewlog.js
> @@ +194,5 @@
> > +  get open() { return this._open; },
> > +  get level() { return 0; },
> > +  get _parent() { return null; },
> > +  get children() { return this._children; },
> > +  getProperties: function() { return ""; }
> 
> Is there a reason why this isn't:
>   getProperties: () => ""
> ?
> 
> I'm not sure if you are doing this conversion by hand or with a script
> applying automatically the rules you listed in comment 1.
> 
> @@ +208,5 @@
> > +  get id() { return this.log.title; },
> > +  get open() { return false; },
> > +  get level() { return this._level; },
> > +  get children() { return []; },
> > +  getProperties: function() { return ""; }
> 
> same here.

Thanks, fixed them :)
I did it with semi-automated macro, and some of them didn't worked (like this, last property), and fixed manually, then I guess I misread that since it starts with "get".

https://hg.mozilla.org/comm-central/rev/6cb35f7f558e
https://hg.mozilla.org/comm-central/rev/eeefdaff1e6c
https://hg.mozilla.org/comm-central/rev/00a06f652415
https://hg.mozilla.org/comm-central/rev/a0cba0d60fcd

Comment 11

[Patrick Cloke [:clokep]]

(In reply to Tooru Fujisawa [:arai] from comment #6)
> im/content/conversation.xml
> -  .forEach(function(a) this.addBuddy(a.buddy, true), this);
> +  .forEach(a => this.addBuddy(a.buddy, true));
> 
> im/content/blist.js
> -  if (contact.getTags().some(function(t) t.id == aTag.id))
> +  if (contact.getTags().some(t => t.id == aTag.id))
> 
> im/content/debug/fake/fake.js
> -  normalize: function(aStr) aStr
> +  normalize: aStr => aStr

Thanks. It makes no sense why they'd keep this syntax for fat-arrow functions and not normal functions.

Anyway, thanks for making all these changes!