Created attachment 266951 [details] [review]
Use modern JS features

Arrow functions: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/arrow_functions

For..of loops: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for...of

Status of ES6 in Firefox: https://developer.mozilla.org/en-US/docs/Web/JavaScript/ECMAScript_6_support_in_Mozilla

TL;DR: gjs has support for both features (since it's based off of the spidermonkey package from Firefox 24) and there is consensus over their designs, ie no big risk that anything will break in the future.

And the reason for doing this:

Arrow functions: shorter simpler syntax than using (function(..) { ...}).bind(this) and you can't forget to bind this since it is bound automatically.

for..of-loops: just shorter and simpler syntax than collection.forEach(...)

Should this  be targeted against the Gjs StyleGuide first?

https://wiki.gnome.org/Projects/GnomeShell/Gjs_StyleGuide

So that Maps does not look totally different from shell/documents/others.
Or do other Gjs projects already make use of these things?

Review of attachment 266951 [details] [review]:

ack

(In reply to comment #4)
> Should this  be targeted against the Gjs StyleGuide first?
> 
> https://wiki.gnome.org/Projects/GnomeShell/Gjs_StyleGuide
> 
> So that Maps does not look totally different from shell/documents/others.
> Or do other Gjs projects already make use of these things?

+1

(In reply to comment #6)
> (In reply to comment #4)
> > Should this  be targeted against the Gjs StyleGuide first?
> > 
> > https://wiki.gnome.org/Projects/GnomeShell/Gjs_StyleGuide
> > 
> > So that Maps does not look totally different from shell/documents/others.
> > Or do other Gjs projects already make use of these things?
> 
> +1

+1 also actually.

I think I'll have a chat with Giovanni, Owen and Jasper and see what they think of this after the freeze (or after 3.12 is out). Thanks for the review guys and sorry for the late reply.

So I actually had this chat with Giovanni sometime this spring. This is the log:

-----------
mattiasb  | ah okay
mattiasb  | Actually one more. :P What's your thought on this:
            https://bugzilla.gnome.org/show_bug.cgi?id=722758
Services  | Bug 722758: normal, Normal, ---, gnome-maps-maint, UNCONFIRMED,
            Use modern JS features
mattiasb  | I did that to just see what the responses would be.
mattiasb  | I was thinking maybe, at some point, one could update the style
            guide and move to using arrow functions and for..of-s gradually.
	    But I'm not sure if people find it worth it
gcampax   | mattiasb: I'm in favor of for..of, and we started using them in
            gnome-shell
gcampax   | because they are the only way to iterate Maps
gcampax   | (but since then we started using them for Arrays too)
mattiasb  | ah nice
gcampax   | as for arrow functions, I don't personally like the syntax
gcampax   | but what's more, they use Function.bind(), which is almost like
            Lang.bind()
mattiasb  | sure, but the semantics is nice.
gcampax   | but puts extra argument at the beginning, instead of the end
gcampax   | so you need to take care when mixing
            Function.bind()/arrow-functions with Lang.bind()
mattiasb  | ah
gcampax   | (and pretty much all existing gjs code uses Lang.bind())
mattiasb  | we're only using Function.bind in maps since quite early
gcampax   | mattiasb: then you're against the style guide already :)
mattiasb  | I know :)
mattiasb  | I think Lang.bind is a bit weird, since it's already available
            in the js spec.
mattiasb  | But I guess it comes from before it existed
gcampax   | mattiasb: yes, we used to support js 181 (firefox 3.6), and that
            didn't have .bind()
mattiasb  | yeah
gcampax   | mattiasb: now Lang.bind() is just a shallow wrapper for
            Function.prototype.bind() - unless you pass extra args, in which
	    case we take the hit for compatibility
----------

I think the take-away here is:
 1) we can move to for..of without much hesitation.
 2) reading this again the main thing Giovanni has against arrow-funcs 
    doesn't make much sense (since arrow-funcs doesn't take a "this"-arg anyways).
    Soo, I don't really know here. I guess "no" for now? 
    (personally I'd say let's go with it since arrow funcs simplify JS 
     "this"-semantics a _whole_ lot.).

(In reply to comment #8)
>
> gcampax   | (and pretty much all existing gjs code uses Lang.bind())
> mattiasb  | we're only using Function.bind in maps since quite early
> gcampax   | mattiasb: then you're against the style guide already :)
> mattiasb  | I know :)

And I told you so. :)

(In reply to comment #9)
> (In reply to comment #8)
> >
> > gcampax   | (and pretty much all existing gjs code uses Lang.bind())
> > mattiasb  | we're only using Function.bind in maps since quite early
> > gcampax   | mattiasb: then you're against the style guide already :)
> > mattiasb  | I know :)
> 
> And I told you so. :)

What?

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > >
> > > gcampax   | (and pretty much all existing gjs code uses Lang.bind())
> > > mattiasb  | we're only using Function.bind in maps since quite early
> > > gcampax   | mattiasb: then you're against the style guide already :)
> > > mattiasb  | I know :)
> > 
> > And I told you so. :)
> 
> What?

I recall I was hesitant about porting all Lang.bind usage to Function.bind because other GNOME/gjs projects use Lang.bind. Anyway, no biggie, we can go back to Lang.bind as easily if we decide to.

(In reply to comment #11)
> 
> I recall I was hesitant about porting all Lang.bind usage to Function.bind
> because other GNOME/gjs projects use Lang.bind. 

Yes I remember. Why bring that up now?

> Anyway, no biggie, we can go back to Lang.bind as easily if we decide to.

Why would we do that?

Any movement on this?

Mattias: Still keen on pursuing this?

Yes!

Since GJS 1.49.90 we also have GObject classes using ES6 classes:

- New API: GObject.registerClass(), intended for use with ES6 classes. When
  defining a GObject class using ES6 syntax, you must call
  GObject.registerClass() on the class object, with an optional metadata
  object as the first argument. (The metadata object works exactly like the
  meta properties from Lang.Class, except that Name and Extends are not
  present.)

  Old:

      var MyClass = new Lang.Class({
          Name: 'MyClass',
          Extends: GObject.Object,
          Signals: { 'event': {} },
          _init(props={}) {
              this._private = [];
              this.parent(props);
          },
      });

  New:

      var MyClass = GObject.registerClass({
          Signals: { 'event': {} },
      }, class MyClass extends GObject.Object {
          _init(props={}) {
              this._private = [];
              super._init(props);
          }
      });

  It is forward compatible with the following syntax requiring decorators and
  class fields, which are not in the JS standard yet:

      @GObject.registerClass
      class MyClass extends GObject.Object {
          static [GObject.signals] = { 'event': {} }
          _init(props={}) {
              this._private = [];
              super._init(props);
          }
      }

  One limitation is that GObject ES6 classes can't have constructor()
  methods, they must do any setup in an _init() method. This may be able to be
fixed in the future.

Created attachment 366903 [details] [review]
configure.ac: Bump GJS dependency

GJS 1.50 is needed for ES6 features.

Created attachment 366904 [details] [review]
Use ES6 arrow notation

Use the arrow notation to bind "this"
to anonymous functions.

Created attachment 366905 [details] [review]
Use ES6 classes

Replace the usage of legacy Lang.Class.
Register classes that needs GObject
functionallity using GObject.registerClass(),
use straight ES6 classes when GObject is not
nessesary.

Created attachment 366906 [details] [review]
utils: Remove unused function

Remove the legacy addSignalMethods function, as all
classes now declares their signals via the
GObject.registerClass() property map.

Attachment 366903 [details] pushed as f035483 - configure.ac: Bump GJS dependency
Attachment 366904 [details] pushed as 173ae44 - Use ES6 arrow notation
Attachment 366905 [details] pushed as acff05f - Use ES6 classes
Attachment 366906 [details] pushed as 342b236 - utils: Remove unused function