Best practices for ES6

[jonathanolson]

I'll come up with some recommendations for how to write our ES6+ style code.

Do's and don'ts.

[jonathanolson]

Instead of var, I've been using let or const. Top-level module imports and constants always should use const. Otherwise it's developer's discretion in my opinion. I tend to write a lot of code where I know whether the variable will be reassigned or not, so I use const when it won't be reassigned (which is the most often).

Generally where possible, I've been using arrow functions instead of anonymous functions, as they make things a lot easier to read (to me). Additionally, it removes the need to have var self = this;, and can turn a lot of 3-line code into 1 line.

E.g.

var self = this;
list = list.filter( function( item ) {
  return item.length === self.desiredLength;
} );

turns into

list = list.filter( item => item.length === this.desiredLength );

For 0-argument functions, I'll use () => something() or () => { return something(); }. For 1-argument functions, I'll use arg => something( arg ) or arg => { return something( arg ); }. For multi-argument functions, parenthesis are required, e.g. ( a, b ) => something( a, b ) or ( a, b ) => { return something( a, b ); }. I'll generally use the braces for something that doesn't fit on one line.

NOTE: Since you can't name an arrow function expression without assigning it to a variable, most recursive functions shouldn't be converted currently, e.g. ( function recur() { recur(); } )();, so there are still some cases where I'll use const self = this;.

For functions defined as "methods" of an object (or just as functions), I'll switch to the ES6 style personally, thus:

var obj = {
  something: function( arg ) { return arg + 2; }
};

turns into

const obj = {
  something( arg ) { return arg + 2; }
};

Since most of my closures and all of my functions on objects turn into different forms, I hardly have any actual usages of the keyword function left.

For looping over an iterable (e.g. arrays):

for ( let item of items ) {

}

For looping numerically, just use let instead of var.

Both of these mean you won't have to "hoist" your var definitions above the loops, and that the variable is locally scoped.

I really haven't run into many cases where I'd want to use variable numbers of arguments, so I can't comment on ...args in the argument list right now. Util.gcd could potentially use it, but it's common code so I didn't convert it (I keep running into cases where I need to GCD multiple numbers, so I have to _.reduce it).

I'm still on the fence about the "options" pattern alternative, particularly in cases where for speed I need to create local variables anyways, e.g.:

options = _.extend( {
  foo: 'bar',
  quantity: 5
}, options );

const foo = options.foo;
const quantity = options.quantity;

vs

const {
  foo = 'bar',
  quantity = 5
} = options || {};

since the latter declares local variables, isolating them in a separate set of statements isn't necessary (which is nice). It would be good to discuss this in dev meeting.

For simpler usages, I've been using default parameters when I know it's very unlikely to have additional options required in the future, or if performance is needed (it's particularly bad for performance to create tons of options objects in inner loops in code). e.g.:

{
  something( foo = 'bar', quantity = 5 ) {

  }
}

For more than 2 defaults, or ANY case where you would want to specify either one but not the other (in this case, quantity but not foo), then the options-style should be preferred unless performance is an issue.

I haven't really used destructuring much, only where you want a duck-typed object without a well-defined type (say you have one function that returns something, and another that uses it), it helps a bit to "declare" the type of it. e.g. say you had a function order( a, b ) { return { min: a < b ? a : b, max: a < b ? b : a } }, then declaring const { min, max } = order( a, b ); can be helpful. I haven't formed a full opinion yet.

I definitely have used array spread/rest operators, which has been super helpful for avoiding ugly array concatenation, e.g.:

const arr = [
  ...getBackItems(),
  middleItem,
  ...getFrontItems()
];

which would be ugly otherwise. And being able to Math.max( ...numbers ) for built-in varargs is nice, but is also useful for more functional programming with apply()/call().

I've also been using string interpolation whenever possible. Instead of 'A + ' + b + ' + C', specifying `A + ${b} + C` has been way clearer. This happens a lot with debug output, toString(), etc., or in non-sim code (e.g. filesystem paths).

I have been using object shorthands (e.g. { a } instead of { a: a }) since it's more convenient, but that usually comes up only in code that's more boilerplate-like.

Async/await is used thoroughly with chipper/perennial code. Many callbacks are async, and if they take no parameters, async () => { ... }. Promise wraps things that aren't using async (but are asynchronous).

Classes have been really nice. Typically I'll declare the constructor at the very top, dropping the @constructor and @extends. I'll also typically add static methods below all non-static ones.

So an example using most features in my preferred style would be:

// Copyright 2018, University of Colorado Boulder

/**
 * Some documentation here, explains what the class/file does.
 * I can now use the word "class" instead of "type" where relevant!
 *
 * @author Jonathan Olson <jonathan.olson@colorado.edu>
 */
define( require => {
  'use strict';

  // modules
  const aNamespace = require( 'A_NAMESPACE/aNamespace' );
  const Util = require( 'DOT/Util' );

  class Something {
    /**
     * @param {number} foo
     * @param {Object} [options]
     */
    constructor( foo, options ) {
      const {
        // {number} - This option does something.
        someOption = 4,

        // {string} - This option can be 'top' or any of the Beatles' proper names.
        otherOption = 'top'
      } = options || {};

      // @public {number}
      this.foo = foo;

      // @private {boolean}
      this.isTop = otherOption === 'top';

      // @private {Array.<number>}
      this.crazy = [
        1, 2, 3,
        ..._.range( 0, someOption ).map( n => this.isTop ? n : -n )
      ];
    }

    /**
     * Simple getter for something.
     * @public
     *
     * @returns {number}
     */
    get doubleFoo() {
      const doubleFoo = 2 * this.foo;
      console.log( `doubleFoo is ${doubleFoo} for ${this.foo}` );
      this.crazy.forEach( crazyItem => {
        console.log( `This is a more complicated statement so it won't fit on one line because of ${doubleFoo}` );
        console.log( 'You should still use single quotes if there is no substitution, according to eslint' );
        console.log( 'But generally don\'t put console.log in getters' );
      } );
      for ( let crazyItem of crazy ) {
        console.log( 'This type of loop is generally preferred for longer closures' );
      }
      return doubleFoo;
    }

    /**
     * Normal method.
     * @public
     *
     * @param {number} quantity
     * @returns {Array.<number>}
     */
    lotsOfFoo( quantity ) {
      return _.times( quantity, () => this.foo );
    }

    /**
     * How to do 2-arg arrow functions.
     * @public
     */
    sortCrazy() {
      this.crazy.sort( ( a, b ) => a - b );
    }

    /**
     * Returns a string representation for debugging.
     * @public
     *
     * @returns {string}
     */
    toString() {
      return `Something[ foo: ${this.foo}, isTop: ${this.isTop}, crazy: ${this.crazy} ]`;
    }

    /**
     * Maybe a static "factory" method.
     * @public
     *
     * @param {number} [halfFoo] - Noted as optional here
     * @param {Object} [options]
     */
    static createSomething( halfFoo = 0, options = {} ) {
      SomethingElse.checkOptionValues( {
        halfFoo,
        somethingElse: 5
      } );
      return new Something( 2 * halfFoo, options );
    }
  }

  return aNamespace.register( 'Something', Something );
} );

I'm curious at what others think of this general style.

[zepumph]

Thank you for the in-depth comment. I have a few questions.

1. Is the following legal es6 to use this to put foo and quanitity on MyType:

MyType( options){
const {
  this.foo = 'bar',
  this.quantity = 5
} = options || {};
}

or maybe self instead?

2. I think that this is a very good list, and it should not just exist in this github issue that will eventually be closed and forgotten. If there are any tweaks to be made, my recommendation is to do so inline to keep them all in one place. From there can we (a) put a link to this comment in Wilder, and (b) update development overview doc regarding these practices?

[samreid]

From there can we (a) put a link to this comment in Wilder, and (b) update development overview doc regarding these practices?

Or document it in phet-info/doc/phet-software-design-patterns.md

[jonathanolson]

Is the following legal es6 to use this to put foo and quanitity on MyType

Not with destructuring. If that's desired, it's possible to do things like _.extend( this, options ). That gets troublesome with passing all options through, and generally isn't desired in most cases.

From there can we (a) put a link to this comment in Wilder, and (b) update development overview doc regarding these practices?

Yes, I figure we can move any "accepted" recommendations to one or both places.

[pixelzoom]

Re the "options" pattern alternative, e.g.:

const {
  foo = 'bar',
  quantity = 5
} = options || {};

I don't see how this works in the context of a class hierarchy, where defaults need to be specified for both subclass and superclass options, then propagated to the superclass. Can you provide an example?

[jonathanolson]

Can you provide an example?

It doesn't seem that great for a hierarchy. You'd have to do:

const {
  foo = 'bar',
  quantity = 5
} = options || {};

super( ..., {
  foo,
  bar
} );

[pixelzoom]

Given #82 (comment), I would be more inclined to stick with our "old" options pattern.

[samreid]

The only time we cannot use ES6 in common code is: we cannot use class for a common code component that needs to be used with inherit.

[samreid]

Consensus: Today we decided new common code files can be written in ES6.

[Denz1994]

Discussion around @jonathanolson comment above:

Instead of var, I've been using let or const. Top-level module imports and constants always should use const. Otherwise it's developer's discretion in my opinion. I tend to write a lot of code where I know whether the variable will be reassigned or not, so I use const when it won't be reassigned (which is the most often).

Going forward we should use let and const. Consider that let remains within the scope it is declared in.

Generally where possible, I've been using arrow functions instead of anonymous functions, as they make things a lot easier to read (to me).

New code should include the use of arrow functions but we don't need to retro fit old sims. Arrow functions should be used when it removes var self. Arrow functions can still be used for self reference in recursive functions.

Classes have been really nice. Typically I'll declare the constructor at the very top, dropping the @constructor and @extends. I'll also typically add static methods below all non-static ones.

Classes can be used for new code but don't retro fit. Object notation for class methods should be used (comma separators are not needed).

For looping over an iterable (e.g. arrays):

There may be some compiling complications that cause files to become too large. Lets stick to using forEach() or traditional syntax if we notice the file getting to large during sim profiling.

@pixelzoom: We can have ES6 in common code and not retrofit old code.

[pixelzoom]

Other notes from 8/23/18 dev meeting:

• For new common code files, use ES6.
• When maintaining a .js file, stick to the style used in the file. If it's ES5, stick with ES5 (unless conversion to ES6 is trivial.)
• Don't convert anything from inherit to class that is used by something that inherits from it.

[zepumph]

in #82 (comment) we got to (but didn't talk about) the paragraph starting with "I really haven't run into many cases where I'd want to use variable numbers of arguments. . ."

[zepumph]

For new common code files, use ES6.

@jonathanolson is there/should there be a difference in practice between scenery (to be thought of as a stand alone graphics library) vs other common code libraries?

[jonathanolson]

@jonathanolson is there/should there be a difference in practice between scenery (to be thought of as a stand alone graphics library) vs other common code libraries?

Seems like we'd want to handle ES5/ES6 in Scenery similarly to how we would do it for other common code.

[jonathanolson]

We finished last time at:

I have been using object shorthands (e.g. { a } instead of { a: a }) since it's more convenient, but that usually comes up only in code that's more boilerplate-like.

[mbarlow12]

I'm noticing that we haven't discussed the spread operator for objects (I may have missed it). But this pattern fails our linting but is valid JS:

const options = {
    1: '1',
    2: 2,
    3: 'three'
};

const otherOptions = { ...options, 3: 'four' }; // { 1: '1', 2: 2, 3: 'four' }

I'm actually having some difficulty finding whether the spread operator in this context has different support than in an array, and it doesn't appear wilder at the moment. Thoughts @jonathanolson ?

[pixelzoom]

@mbarlow12 I can't speak to why your example is failing lint. But specific to options, we decided to stick with the _.extends pattern. See #82 (comment), #82 (comment), and #82 (comment).

[mbarlow12]

@pixelzoom it's slightly different. #82 (comment) uses the spread operator to duplicate all key-value pairs, allowing the user to override or add new keys/values to the resulting object. #82 (comment) uses destructuring to assign values to const foo and const quantity either from defaults or options.foo and options.quantity, respectively. An alternative to #82 (comment) would be:

const superOptions = {
  foo: 'bar',
  quantity: 5,
  ...options
};
super( ..., superOptions );

Above, options.foo will override 'bar' (note that placing ...options first will allow the defaults to override). Admittedly, this assumes that you want all keys in options to be passed to the call to super.

This could be more useful in nested subtype options:

const subtypeOptions = {
  foo: 'bar',
  quantity: 5,
  ...options.subtypeOptions
};

const subtype = new Subtype( arg1, arg2, subtypeOptions );

or

const subtype = new Subtype( arg1, arg2, { foo: 'bar', quantity: 5, ...options.subtypeOptions }

This specific use of spread operators in object literals hasn't been discussed here or in wilder, so I thought it'd be useful to bring it up.

[samreid]

@mbarlow12 thanks for pointing that out, it seems spread operator for objects is a nice alternative to lodash extends. But it was unclear to me that spread operator for objects is formally ES6--MDN reports this as "new in ECMAScript 2018" (beyond ES6): https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Spread_syntax . I don't know whether eslint or Babel transpiler support it.

[jonathanolson]

I recall checking this and that babel didn't support that feature.

[mbarlow12]

It appeared as though the 'stage 4' proposals had been implemented, but I don't see support from Babel. I may have been confusing Babel's JSX/React capabilities with what it can do for vanilla JS.

It also looks like Object.assign does the same thing. Thanks!

[mbarlow12]

I think @samreid asked about some additional details on async/await last week. One note I'd wanted to make was that await can be placed in front of pretty much anything and will coerce non-promises into resolved promises

let i = await 20

is the same as

let i = await Promise.resolve(20);

await will halt execution until the promise resolves or the function returns, so the placement of await statements can have pretty significant impacts on execution time.

  const asyncExample = async () => {
    const a = new Promise( ( res, rej ) => {
      setTimeout(function() {
        res( 'a resolved\n' );
      }, 2000);
    } );
    const b = new Promise( ( res, rej ) => {
      setTimeout(function() {
        res( 'b resolved\n' );
      }, 3000);
    } );

    console.log( await a, await b );
  }

  const asyncExample2 = async () => {
    const a = await new Promise( ( res, rej ) => {
      setTimeout(function() {
        res( 'resolved' );
      }, 2000);
    } );
    const b = await new Promise( ( res, rej ) => {
      setTimeout(function() {
        res( 'b resolved' );
      }, 3000);
    } );

    console.log(  a,  b );
  }

asyncExample2() will take 5 seconds to run while asyncExample() takes about 3. The MDN docs also have a more detailed example and discussion.

[samreid]

We'll discuss this at Core Hours in 2 weeks. @zepumph will add it to the calendar.

[jonathanolson]

Decided we can postpone the register for ES6 classes until after statics are assigned:

e.g.:

class SomeClass {
  
}

SomeClass.VALUE = 5;

return namespace.register( 'SomeClass', SomeClass );

UPDATE: @zepumph and @samreid discussed this 1/18/2019 and, with help from phetsims/wave-interference#281, decided that this pattern for statics works well for attributes, but you can still use the static keyword for methods in your class.

[samreid]

@jonathanolson did we conclude this issue can be closed?

[jonathanolson]

@jonathanolson did we conclude this issue can be closed?

Yes, but will be good to mention the for-of loop issue during the next meeting so I'll leave it open.

[Denz1994]

Devs are no longer reviewing this issue weekly. Safe to close.

[pixelzoom]

Discussed for-of at 12/13/18 dev meeting.