Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial ES6 style guide #276

Merged
merged 17 commits into from
Apr 29, 2015
Merged

Initial ES6 style guide #276

merged 17 commits into from
Apr 29, 2015

Conversation

goatslacker
Copy link
Collaborator

A mostly reasonable approach to ES6

Don't want to read the updates in diff form? Read the ES6 style guide in full here.

Introduction

Welcome to the future. This PR updates our style guide to reflect the current style for writing ES6 at Airbnb. As before, this is a living document and it'll change over time as we and the community learn. Of course, this isn't the "one true way to write ES6" and doesn't intend to be—just one mostly reasonable approach.

We think having a style guide is important for readability of code and scalability of engineering teams. Our philosophy is that you shouldn't be able to tell who wrote a file based on the style of the code.

As always, please fork this guide and use it as a starting point for your own style guide, and if you hit on a problem or something you'd like to share, we welcome Issues and Pull Requests—we love having discussions about this stuff.

What's New

We've updated existing sections to reflect ES6 best practices (i.e., var -> const) and added guides for the following language features:

ES5 Support

We'll continue to maintain the ES5 style guide at /es5/README.md. We moved the ES5 style guide into its own folder because Airbnb is moving towards ES6, and since this public repo is the style guide we use internally, we'd like it to reflect our current best practices. We'll also be adding additional style guides that we use internally, like our in-progress React+JSX style guide.

Learn ES6

Some resources we've found helpful in learning ES6:

Compiling ES6

If you'd like to use ES6 syntax in a reasonable set of current browsers, you'll have to transpile your code to ES5. We recommend both traceur and babel as activley-maintained, standards-compliant ES6 compilers. We use Babel internally.

FAQ

What happened to the old guide?

It's still here! Have a look: /es5/README.md

Should I be writing ES6?

Up to you and your team! We're making the switch at Airbnb. Check out our ES5 style guide if you're not making the jump yet.

Why does JavaScript need a style guide?

A great question, see: #102

Next Steps

The plan is to merge this after a week or so of gathering feedback from the community. Have a look and let us know your thoughts.

Read the ES6 style guide

Special thanks go out to @iamnirav, @alvinsng, @hshoff, @justjake, and @spikebrehm for looking at a draft of this.

Fixes part of #247.

🍔 🍟

@hshoff
Copy link
Member

hshoff commented Apr 2, 2015

the-science

@hshoff hshoff mentioned this pull request Apr 2, 2015
3 tasks
@danielbayerlein
Copy link

😍

@mfunkie
Copy link

mfunkie commented Apr 2, 2015

Excited for this! Along with the forementioned React + JSX style guide.

@meenie
Copy link

meenie commented Apr 2, 2015

Thumbs Up

@alvinsng
Copy link
Contributor

alvinsng commented Apr 2, 2015

tim-and-eric-mind-blown

@iamnirav
Copy link
Contributor

iamnirav commented Apr 2, 2015

Awesomesauce. Thanks for leading the charge, @goatslacker!

approval-thumbs-up-gif-36

class Jedi {
constructor(options = {}) {
const lightsaber = options.lightsaber || 'blue';
this.set('lightsaber', lightsaber);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could real getters and setters be used here?

class Jedi {
  constructor(options = {}) {
    this.lightsaber = options.lightsaber || 'blue';
  }

  get lightsaber() {
    return (this.team === 'rebel') ? 'blue' : 'red';
  }

  set lightsaber(ls) {
    this.team = (ls === 'blue') ? 'rebel' : 'empire';
  }
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Ross!

I'm guessing we left that out of the style guide because getters & setters must be supported by the runtime, and can't be transpiled to ES5.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They predate ES6 (ES5.5?). They're g2g in IE9+. Babel compiles them to Object.defineProperty calls since it looks like IE8 doesn't support the get and set syntax.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Getters and setters can be used here. However, I personally have a strong opinion against them.

It's possible to write code that does:

class Jedi {
  constructor(options = {}) {
    this.lightsaber = options.lightsaber || 'blue';
  }

  get lightsaber() {
    return (this.team === 'rebel') ? 'blue' : 'red';
  }

  set lightsaber(ls) {
    while (true) { }
  }
}

which will then fail when you do:

const jedi = new Jedi();
jedi.lightsaber = 'blue';

While the same (writing really stupid code) is true for methods, at least the methods are explicit (when read) in that an action will take place when they're called.

Having side effects happen when you set a property on a class is a code smell IMO -- just side effects in general are meh.

That said, thanks for pointing out this example. Setting properties on this using strings is a little strange. Ideally we'd have JS be strongly typed, not stringly typed o_O.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree misusing getters and setters can lead to code that is less readable than not using them, but that doesn't always have to be the case. I mentioned this because it seems odd to suggest explicit get and set methods when the language supports defining getters and setters for object properties already.

This was here before this PR though I think, so it's probably worth chatting in another thread. The work on ES6 looks great 👍.

}

toString() {
return 'Jedi - ' + this.getName();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return `Jedi - ${this.getName()}`;

@goatslacker
Copy link
Collaborator Author

I removed trailing commas from examples but they should be ok since babel and traceur both remove them when transpiling.

if (currentUser) {
test = function test() {
test = () => {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test = () => console.log('Yup.');

@RobLoach
Copy link
Contributor

RobLoach commented Apr 6, 2015

http://i.imgur.com/xsEP5iY.gif

@hco
Copy link

hco commented Apr 7, 2015

The plan is to merge this after a week or so of gathering feedback from the community
Awesome work & promising schedule! 😃

From my point the linters are still kind of a blocker, as they are still mentioned in the README, but https://github.com/airbnb/javascript/tree/es6/linters does not represent these changes.
IMHO they should be adjusted or marked as deprecated, prior merging.

Anyway: Thanks for your work! 👍

console.log(declaredButNotAssigned); // => throws a ReferenceError
console.log(typeof declaredButNotAssigned); // => throws a ReferenceError
const declaredButNotAssigned = true;
}
```

- Anonymous function expressions hoist their variable name, but not the function assignment.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't true for let. The TDZ still applies; it doesn't matter if the initial value is an function.

 console.log(anonymous);

will throw a ReferenceError.

Same with named function expressions below.

/cc @banderson

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice find. This is meant to be var and is a victim of some serial regex replace.

@justjake
Copy link
Collaborator

Now that Harry has added an eslint rc, can we merge this bad boy?

@goatslacker
Copy link
Collaborator Author

Soon-ish. Todo:

  • Go through all the feedback provided.
  • Amend changes where necessary.
  • Add some resources for learning ES6.
  • Rebase with master.

goatslacker and others added 6 commits April 28, 2015 21:27
I found the term "function constructors" to be confusing as the functions the section refers to are not constructors. I think "higher-order functions" or something similar would be more clear.

Thanks!
@goatslacker
Copy link
Collaborator Author

@hshoff ready to go. 🎈

@justjake
Copy link
Collaborator

🎉

On Tue, Apr 28, 2015, 9:33 PM Josh Perez [email protected] wrote:

@hshoff https://github.com/hshoff ready to go. [image: 🎈]


Reply to this email directly or view it on GitHub
#276 (comment).

@hshoff
Copy link
Member

hshoff commented Apr 29, 2015

@goatslacker
yasss

goatslacker added a commit that referenced this pull request Apr 29, 2015
A mostly reasonable ES6 style guide
@goatslacker goatslacker merged commit 1df3160 into master Apr 29, 2015
@hco
Copy link

hco commented Apr 29, 2015

Awesome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.