Node v6 style guide

[hueniverse]

It's time to review the upcoming features in node v6 and make any changes necessary to the style guide.

@cjihrig / @Marsup - care you give it a try again?

My plan is to move hapi core to use v6 as soon as it is out as the primary platform, and keep supporting v4 as LTS. Node v0.10 support ends June 1st.

[mark-bradshaw]

I'm not sure what all new features we can expect, but the only one I can think of is the spread operator.

[mark-bradshaw]

I guess we get new.target as well.

[cjihrig]

@mark-bradshaw Node v6 is actually coming with almost all of ES6 supported (see http://node.green/). The hard part is identifying the useful stuff vs. the crap, taking performance into consideration (see https://kpdecker.github.io/six-speed/), and deciding on style rules.

[mark-bradshaw]

Whoa. That's a lot more green than 5!

[arb]

@hueniverse is there anything that you're super for or against?

[hueniverse]

@arb nope. I'm open to anything that's properly optimized and makes things clearer or simpler.

[geek]

Default Parameters

Should have a single space between the =.

const fn = (isParam = true) => { };

What about if you assign an object to the parameter, how would you space that?

const fn = (options = {
  isParam: true,
  foo: 'bar'
}) => {

    if (options.isParam) {
    // do something
    }
};

Or require it as a separate variable?

const defaultOptions =  {
  isParam: true,
  foo: 'bar'
};

const fn = (options = defaultOptions) => {

    if (options.isParam) {
    // do something
    }
};

[Marsup]

That's not the same behavior, those are not cloned, in the 2nd case if you modify it the next default won't be the same.

[geek]

@Marsup what am I doing wrong here:

'use strict';

const defaultOptions = {
  foo: 'bar'
};

const fn = (options = defaultOptions) => {
  console.log(options);
};

fn();
fn({ foo: 'hello' });
fn();
fn({ bar: 'foo' });
fn();

outputs:

{ foo: 'bar' }
{ foo: 'hello' }
{ foo: 'bar' }
{ bar: 'foo' }
{ foo: 'bar' }

[cjihrig]

What are you expecting?

[geek]

Was trying to give an example of what @Marsup was saying, wasn't clear to me.

[Marsup]

The keyword was "modify" ;)

Try that :

const fn = (options = defaultOptions) => {
  console.log(options);
  options.baz = 'qux';
};
fn()
fn()

[geek]

@Marsup I was modifying on the otherside, thanks... updated:

'use strict';

const defaultOptions = {
  foo: 'bar'
};

const fn = (options = defaultOptions) => {
  options.bag = 'boy';
  console.log(options);
};

const fn2 = (options = defaultOptions) => {
  console.log(options);
};

fn();
fn({ foo: 'hello' });
fn({ bar: 'foo' });
fn2();

outputs:

{ foo: 'bar', bag: 'boy' }
{ foo: 'hello', bag: 'boy' }
{ bar: 'foo', bag: 'boy' }
{ foo: 'bar', bag: 'boy' }

In that case, we should add the following rule:

Default parameters

• Default parameters should always be assigned using a cloned object or a new object in the assignment

// Right
const fn = (options = {
  isParam: true,
  foo: 'bar'
}) => {

    if (options.isParam) {
    // do something
    }
};

// Wrong
const defaultOptions = {
  isParam: true,
  foo: 'bar'
};
const fn = (options = defaultOptions) => {

    if (options.isParam) {
    // do something
    }
};

// Right
const defaultOptions = {
  isParam: true,
  foo: 'bar'
};
const fn1 = (options = Hoek.clone(defaultOptions)) => {

    if (options.isParam) {
    // do something
    }
};

const fn2 = (options = Hoek.clone(defaultOptions)) => {

    if (options.foo) {
    // do something
    }
};

[arb]

I don't think we'd want to even include the Hoek.clone option. That's an expensive operation to do.

[geek]

@arb agreed, wanted to provide options here. So, only assign new objects if assigning an object. Everything should be inlined.

[kpdecker]

I've personally had a number of issues with the distinction between how default options are handled for the no object case vs. the empty object case. i.e.

const fn2 = (options = {foo: 1, bar: 1}) {};
fn2();
fn2({bar: 2});

In the later case, you are silently resetting foo from the "default" to undefined. Usually I intend to only change bar and maintain the default behavior of foo, but this is not what happens. As a consequence of this, I've moved away from using defaults for objects for anything other than (options = {}) and then implement the default behaviors via other mechanisms, for each flag/key.

[kpdecker]

Have added six-speed numbers for Node 6. http://kpdecker.github.io/six-speed/

[cjihrig]

Based on the numbers, I'm hugely in favor replacing arguments with rest params. The only style question is spacing around .... I'd vote , ...arg).

[cjihrig]

@hueniverse there are a lot of ES6 features, but other than rest params, nothing I feel very moved by. A few notes:

• Destructuring still seems slow. Combined with the endless variations, -1 from me.
• The spread operator still seems a bit slow but makes the code cleaner. +1 from me outside of hot code paths.
• So far, I haven't been a huge fan of default params, but I'll defer to others there.
• I don't remember if we adopted template literals when we moved to v4, but I think we should allow them.

I'd also like to hear from others on features that they like/dislike.

[mark-bradshaw]

Thanks for the numbers @kpdecker. Based on that rest params does seem like a huge win, and I think it would be a good addition. I mostly agree with Colin, except I do like destructuring. I think it often makes code easier to read and shorter. I'm in favor. Otherwise, I like spread, simple use of default params, and really like template literals.

[hueniverse]

I would allow but not require features that have no negative performance impact. We should list them.

Can we just completely disallow arguments with a lint rule?

[cjihrig]

Can we just completely disallow arguments with a lint rule?

Yes, http://eslint.org/docs/rules/prefer-rest-params.

[cjihrig]

Based on the latest six-speed numbers, these are the things that aren't currently in the style guide, which don't come with a performance hit:

• Rest parameters (1.2x faster)
• Template strings (identical)

These are the things that aren't currently in the style guide, which still have performance issues:

• Classes (1.2x slower)
• Default parameters (2.6x slower)
• Destructuring (1.3x to 14x slower)
• for...of (2.1x to 8x slower)
• Generators (5x slower)
• Map and Set (several measurements, but generally slower)
• Object.assign() (5x slower)
• Object literal computed properties (9x slower)
• Spread operator (3x to 35x slower)
• Template string tags (2.8x slower)

[devinivy]

Template strings are good as long as they don't contain complex expressions– I think we already use/prefer them specifically when concatenation would need to be used with single-quotes. Otherwise single-quotes are preferred.

Also, we already use Map/Set now (at least in hoek). I think they can be useful– you know when you need it and when you don't. I also don't think usage of Map/Set really needs to go in the styleguide.

Classes are fine. The performance hit is fairly superficial, given that classes aren't generally created in hot code-paths. They are currently used in hapi sparingly.

[hueniverse]

I would call out features that should not be used in "runtime" code vs "setup" code. For example, today we allow optional arguments only in setup code but not in runtime code (hence methods that require an empty options argument).

[devinivy]

I suggest we just choose the features that we want to allow then make notes about the ones that should not be used in runtime code, along with what their more performant alternatives are. Over time (as the features become more performant) hopefully we'll be able to remove those notes. In the case of classes, we probably don't even need to make such a note.