Proposal for allowed ES6 features in hapi

[cjihrig]

Now that hapi 10 is out and using Node 4, we need to consider which ES6 features (and general JS features) should be allowed in hapi modules, as well as style guide issues, etc. To get the discussion going, here is a list that @arb and I came up with.

Strongly Recommend

Strict Mode

Strict mode makes JavaScript more restrictive. It prevents the use of several features that are considered bad practices. It also enables certain new language features. In the past it has been known to help V8 (unsure if this is currently true as V8 changes rapidly). It is trivial to enable. Node core has moved to strict mode everywhere.

Object.assign()

Object.assign() is the preferred way to copy/merge objects. This can be used as a replacement for foo || {} (simpler code coverage), as well as several Hoek methods.

const

const is used to define constants. const can be used to declare a surprisingly large number of variables that are not what would normally be considered "constants." For example, in Node core, it has become a convention to use const foo = require('foo');. In general, const should be favored and used in any case where a variable’s value doesn’t change.

Map and Set

Node now offers proper map and set data structures. According to six-speed the performance is better than the ES5 alternatives. Maps are preferred over using objects as key-value stores (although this can present problems when returned from public APIs, but works perfectly fine for internal APIs).

New Math and Number Methods

New methods such as Number.isInteger() make tasks like checking for an integer, much more readable. Methods like Number.isNaN() provide a stricter alternative to the global isNaN().

Nice to Have

Template Strings

Template strings are used as an alternative to string concatenation and often require a few less characters. Performance may still be slightly worse according to six-speed. This is currently used in Node core.

Classes

Classes are syntactic sugar. They currently require strict mode. One thing worth noting is that six-speed indicates super() performs poorly.

Numeric Literals

The old way of declaring octal literals is forbidden in strict mode. The new octals, as well as binary constants, are more intuitive. This is probably not very common in the hapi ecosystem, but should be allowed, if needed, for readability.

Use with Caution

Object Literal Shorthand and Computed Properties

If deemed suitable, these should be allowed. However, computed properties were broken until relatively recently. According to six-speed, there is a significant performance penalty (although a more fine grained breakdown of shorthand vs computed properties would be nice).

Arrow Functions

Arrow functions do not clobber the existing this value, making them very useful as callback functions. However, there are a number of ways to burn yourself with them:

• Their behavior changes slightly depending on their syntax. Strongly recommend enforcing the use of both () and {}.
• Anytime this is important. For example, when using this inside of an object literal.
• Arrow functions throw when used as the right hand side of the instanceof operator.

let

let provides block scoping, and in the general case is comparable to var. However, there are some cases where it breaks down. See Trevor Norris's comments here.

Things to Avoid

Symbols

Symbols allow you to more effectively hide data. I haven't encountered problems with them, but also don't see a real need for them.

General

Anything that is still behind a flag. These features are generally not complete, hence the flag. If you run into trouble, you will be sent to the V8 issue tracker.

References

• https://kpdecker.github.io/six-speed/

[gergoerdosi]

I would also like to add promises to this list. Hapi uses callbacks everywhere. I'm not saying module developers should completely switch to promises, but would be good if functions returned a promise when a callback is not passed to arguments.

[cjihrig]

That would probably require a massive rewrite of the entire hapi ecosystem in order to be consistent. Not saying that's a bad idea, but gmail marked your message as suspicious :-)

[hueniverse]

We can allow exposing a promises API but not using promises internally in hapi modules.

[kpdecker]

One comment on map/set:
The lookup case is faster. The assignment case is slower. Looking at the tests I also appear to have missed the simple read case for the lookup that could vary some. I'll add this on the next cycle of the tests (hopefully in the next day or two pending some automation work)

[danielb2]

Symbols allow you to more effectively hide data. I haven't encountered problems with them, but also don't see a real need for them

For a thing to avoid, I think that provides a pretty weak argument. A performance penalty would be justification to avoid something. Seems Symbols should simply be left to the discretion of the implementor. In fact, for implementations where you really want something to be hidden from the public interface, Symbols provide a great solution.

[cjihrig]

This is all open for discussion. I questioned putting Symbols in the things to avoid list. I just don't see them as that useful because Object.getOwnPropertySymbols() still lets you access the "hidden" data.

[AdriVanHoudt]

I really like the list, maybe when there is an agreement add some rules to eslint from lab?

[salzhrani]

Would love for hapi server methods to return promises

[connor4312]

A 👍 from me for returning promises. Especially with ES7 async functions, promises seem the way of the future. I think it would also be very nice to see internal use of let, const in the future, once Node fully supports them. They reduce potential errors and make code easier to reason about. Same for classes, once full support arrives. Hapi is pretty standard internally, but for an unfamiliar developer it's easier to see a standard class declaration than figure out which of the thousand and one ways to create, compose, and extend classes some particular module uses.

[Marsup]

Agreed on most of it.
I wouldn't have such a strong stance on arrow functions about the use of {} or (), been using it for a long while in various forms, all are useful and readable.
I don't see any use for Symbols either but it would be bad to ban it unless there's a problem with it.

To the last few comments, this is a language feature debate, please stay on topic, hapi's API (or any other module) should be discussed there, this is not the place.

[dsernst]

Object.assign()... can be used as a replacement for foo || {} (simpler code coverage)

@cjihrig: Can you clarify this?

[cjihrig]

var bar = foo || {} requires two separate tests, one where bar = foo, and one where bar = {}. Object.assign({}, foo) requires a single test.

[hueniverse]

Aren't two tests better here?

[AdriVanHoudt]

If the tests are there I see no point in removing them. If not testing if Object.assign works seems redundant.

[cjihrig]

I'm not advocating for removing existing tests, but for new code it requires writing one instead of two.

Aren't two tests better here?

I don't think so. Object.assign() automatically handles falsey values for you. It's the same idea as Hoek.reach() simplifying testing the pattern foo && foo.bar (I know that's not the intent of Hoek.reach()).

[hueniverse]

My point is, there are two options here and you are hiding one of them from the tests.

[arb]

Maybe some code will help clear up what @cjihrig is talking about...

var hoek = require('hoek');

var defaults = {
  foo: 'bar',
  protocol: 'http'
};

var register = function (options) {
  var hoekApply = hoek.applyToDefaults(defaults, options || {});
  var assign = Object.assign({}, options, defaults);
}

register({
  foo: 'baz'
});

With this code, hoekApply and assign have the same values and you don't need the extra || test. Tests are good, but tests just to cover the || check for optional options arguments get really tedious. Object.assign lets you get around that problem because the logic is down a level at the JS layer.

[AdriVanHoudt]

that makes it clear, thx

[hueniverse]

Again, you are making the argument that testing both cases of options isn't useful and I am disagreeing. Just because you are hiding the condition doesn't mean it's not there anymore.

[kpdecker]

Added an updated test for string-keyed value reading from Map objects to six-speed. Looks like Map is about half the speed for that use case. For my own personal projects, I'm only planning on using Map for rare case that I have non-string key values unless that changes.

[cjihrig]

Simplified coverage (if you want to call it that or not) is just a side effect of Object.assign(). It's also nice because it gives you the expected object type in the case of something like foo || {}, where foo is 5 instead of the expected falsey or object type. This is obviously a programmer mistake, but it makes your code a little more robust. Then, there is the usage for cloning objects.

What are the next steps for moving forward with this?

[hueniverse]

A PR on the style guide as well as a new section for allowed language features. Also, are we going to recommend always using let vs var vs const?

[AdriVanHoudt]

Maybe also propose some eslint configs for the new styles, it will make the transition easier. @cjihrig I can make a PR on the config repo with the ones I added which seemed sane to me (and ofc in line with this discussion) if you want
Imo you can only use const in strict mode otherwise it does not even error on reassign.
@hueniverse do you mean a new section in the style guide with allowed es6 stuff?

[hueniverse]

This list says what you can use, so we should probably list stuff you should not

[AdriVanHoudt]

Seems reasonable :P

[hueniverse]

I would like to hear what people think about the arrow function rule (require both () and {}). This seems to be going against the trend of keeping things short for a lot of the simple cases. Can we map the pitfalls of omitting them and agree on the limits we want to enforce?

[AdriVanHoudt]

I prefer requiring both, the auto return and stuff can be confusing. If you really want this mapping the allowed cases and putting them in the style guide is fine.

[Marsup]

These are the rules I try to follow for my projects :

• enforce arguments parentheses
• single statements or conditionals with short-circuit allowed as long as they don't span over more than 3 lines
• parentheses around object creation allowed ((foo) => ({ bar: foo }))
• no function creation with implicit return (() => () => 42)

I strongly disagree that implicit returns reduce readability, quite the opposite.

[AdriVanHoudt]

This is just a matter of opinion :P I just like explicit returns not that leaving them out reduces readability per se, just my preference.
The rest I can agree on.

[toboid]

@Marsup could you expand on your second point please? Not quite sure what is meant by that.

I find implicit return more readable and it communicates the intention to just return a value as opposed to perform some other logic first.

[arb]

I agree with the rule as stated. It's much easier to remember a single rule than to try to remember three. Also, if you opt for the short syntax, you had to make code changes just to put a console.log or debugger statement in the body, then remember to undo it before submitting a PR. There is also the implied return you have to remember is there as well which I am not a fan of.

I agree with @cjihrig; if we start dropping {} off function bodies, soon we'll start dropping {} from single line if statements.

[Marsup]

You already have to remember a lot of rules, oh wait... there's eslint for that ;)
Yes when you modify the code you actually have to be careful about what you're doing, your point being ?
I fail to see the relationship with if.

I'm just reporting what I actually use after almost 2 years of practice with it, agree to disagree ;)

[cjihrig]

agree to disagree

Agreed. Style guides are like religion, sports, and politics.

Serious question though. Are there existing ESLint rules for the items in your list other than arrow-parens?

[arb]

My point being, I should not have to add {}, log something, make the real change, then remember to take those {} back out when I'm done. They should just be there.

The relationship with if is that JS technically supports a terse syntax that introduces the same annoyances omitting {} on functions does. If we let terse syntax on one thing, people are going to want it everywhere it's supported, despite good judgement.

[Marsup]

No other rules out of the box, eslint's support for ES6 is fairly recent so... That didn't prevent us from creating our own.

If you want to add statements and forget about the braces that's OK, I'm not advocating for banishing them, just allowing the other syntax.
I don't think such behavior would spread on if, and anyway we have rules for that.

[hueniverse]

We already have rules people find odd. I am not worried about that. But rules should enhance the code and I think some arrow function shortcuts make the code more readable.

I agree with these:

• enforce arguments parentheses
• parentheses around object creation allowed ((foo) => ({ bar: foo }))
• no function creation with implicit return (() => () => 42)

I am not sure about:

• single statements or conditionals with short-circuit allowed as long as they don't span over more than 3 lines

Here are my concerns: first, it is not a well defined rule. It's more about "feeling" and that's never a good idea for a community project. We also don't have line length limit and I don't want to introduce one. We can make a single statement single line rule and this will self-police itself as crazy long lines will just because unreadable and people will break them up and add {}.

But the real issue is that we already have a hard rule for how to write functions. We do not allow single line functions because we want the visual cue of the empty line to call your attention you are switching context and potentially event loop tick. That's what I am worried about losing.

The context is no longer an issue because of how this is set in arrow functions. The next tick concern still stands. That said, I like the readability of (a) => a + 5.

[nlf]

i'm a +1 for allowing single statement implicit returns. there are a lot of scenarios where it's a lot simpler and where a block is really not necessary. i think if your function body is a single statement, then go for it.

[hueniverse]

@nlf single statement and single line?

[devinivy]

Could we require an empty line whenever a tick is possibly at stake? Do our rules require that they be fully enforceable by eslint?

[nlf]

preferably single line, yes. i agree with you that people will end up policing themselves on extremely long lines for their own sanity, so one statement one line makes sense

[hueniverse]

@devinivy no one will be able to keep track of that. I think it's ok to not go there and just start simple and see what the code looks like moving forward.

[devinivy]

I expect we'll find that single statement arrow functions rarely will be used in cases that a tick is at stake anyway.

[nlf]

i agree, i don't think a single statement is going to be asynchronous often at all

[hueniverse]

We should also allow ({}) to span multiple lines.

[hueniverse]

Conclusion:

• enforce arguments parentheses
• parentheses around object creation allowed ((foo) => ({ bar: foo })) including line breaks in the {}
• no function creation with implicit return (() => () => 42)
• allow single statements on single line to use implicit return without {}

[cjihrig]

@Marsup (or anyone else), do you have any custom ESLint rules you'd be willing to share to help enforce this (specifically items #3 and #4 in the previous comment)?

[Marsup]

Sorry, nothing yet.

[Marsup]

I'm tinkering with template strings on joi, it brings a bit more readability. I've tested it against node 5.0 and it's still 2x slower, but it's an unlikely scenario of massive concatenations in a loop, I doubt it would impact the project in any significant way. Do you have thoughts about it ?

[cjihrig]

Personally, I prefer template strings over string concatenation for readability. The only tricky part would be enforcing when to use them. Always? Only in place of concatenation?

[AdriVanHoudt]

how would you deal with splitting strings up on multiple lines when to long?

[Marsup]

@cjihrig it probably adds extra pressure on the VM so only where it makes sense.
@AdriVanHoudt I don't like it at all, it messes your indentations.

[AdriVanHoudt]

@Marsup not encountered that problem (yet)

[willin]

async generator