-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Add JavaScript style guide #302
Conversation
|
||
* Prefer ES6 classes over library provided classes. | ||
* Prefer `===` and `!===` to `==` and `!=`. | ||
* Avoid using arrow functions when not necessary. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any more guidance you can can provide to clarify "when necessary" or why to avoid? Is it just for binding this
with fat arrow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've seen a few people take advantage of =>
being shorter which makes code misleading since you assume it will be using the defining scope's this
value.
Avoid using arrow functions when not using the defining scope's
this
value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see the value in being explicit about using this
, but I don't like giving up on the clarity arrow functions can provide with implicit return and shorter syntax
users.map(user => user.name)
// vs
users.map(function(user) { return user.name })
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
which makes code misleading since you assume it will be using the defining scope's this value.
Is this true? I don't make that assumption. I've worked on projects where the rule was to simply always use the fat arrow. For some reason I always thought that was a bad idea but I have never actually ran into a time where it caused an issue.
Is there ever a case where this would actually cause a problem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a technical problem, but I like being able to visually distinguish the two.
@christoomey That is an edge case to the rule and I would definitely prefer to see that.
Any ideas on how we can word this and make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides a tiny bit more code generated when transpiled, no.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm in favor of changing this to always use the fat arrow then. That prevents possible confusion over things not working as expected and it is one less thing to have to worry about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer arrow functions
=>
, over thefunction
keyword
Better wording?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's an arrow function? Are there docs?
How do we feel about something like https://twitter.com/raganwald/status/564792624934961152
|
That has a very Swift/Rust/Scala feel to it. I'm not opposed to it, but I'm curious how others feel about it. |
@@ -183,7 +183,9 @@ Email | |||
JavaScript | |||
---------- | |||
|
|||
* Use CoffeeScript. | |||
* Use ES6 when possible with a tool like [6to5]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about using "Prefer", since we define it in the README?
- Prefer ES6 with a tool like 6to5.
- If 6to5 is not possible, use CoffeeScript (?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, a best practice won't change often (for example, "avoid global variables" and "keep classes small"). Will 6to5 stand the test of time? Or is it just a current solution?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The best practice is Use ES6
, I just point to the best solution (I know of) currently instead of keeping it ambiguous.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use ES6 when possible with a tool like 6to5, otherwise use CoffeeScript
Better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sounds like it's changing ES6 to our default in projects (over coffeescript). Is that what we've determined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like it to be a default, but Use CoffeeScript
isn't really the answer anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have an ongoing experiment related to ES6: https://trello.com/c/EGpNLWB1/393-es6-instead-of-coffeescript
It seems like the winds are blowing this way, but I think we should try out ES6 on a JavaScript-heavy project before determining it's ready to replace Coffeescript on projects going forward.
I think it would be fine to get guidelines in place for HOW to use ES6 in the event that you are using it. I just think we should have a JavaScript ES6 section instead of replacing the existing Coffeescript recommendation.
It could also be reasonable to soften this to something like, "Use Coffeescript, ES6 with 6to5, or another language that compiles to JavaScript." Basically, make the stance that JavaScript is a compile target.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a fan of "Use Coffeescript, ES6 with 6to5, or another language that compiles to JavaScript.". I think it does a good enough job of saying use JavaScript but avoid ES5.
Is it too soon to have an ES6 styleguide? Do we have enough experience with it? How does it compare to the community's styleguide? |
I feel like I've written enough ES6 to at least start a guide. I think we'll end up adding/changing it as more of us use ES6. |
[Sample](sample.js) | ||
|
||
* Prefer ES6 classes over library provided classes. | ||
* Prefer `===` and `!===` to `==` and `!=`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
!==
, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch, pushed a fix.
@BlakeWilliams What do you think about including JSHint dependency to this JS style guide? It might be even good to provide a |
Would that go under best practices? I can definitely see JShint being a best practice. Also, I'm not sure if we should include a jshintrc since it's sometimes environment dependent.https://github.com/jshint/jshint/blob/master/examples/.jshintrc |
My thoughts around providing tool configs, is that a lot of folks that want to follow our style guide could just grab the config file, plug it into their project and have automated checks against thoughtbot style guide. I've had a client request thoughtbot's RuboCop config file, and I think this could helpful across the board. |
Would it make sense to point them towards Hound in those instances if applicable? I think a lot of our guides aren't really enforceable with tools like jshint. eg: Use |
* Prefer [template strings] over string concatenation. | ||
* Prefer promises over callbacks. | ||
* Prefer array functions like `map` and `forEach` over `for` loops. | ||
* Use `const` to declare variables. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since nothing in JavaScript is truly immutable (except for numbers), I wouldn't go so far as to use const
for everything -- perhaps just for constants, i.e. things in SCREAMING_SNAKE_CASE. However, I would use let
over var
-- the block scope works like Ruby so it's familiar, the block scope also prevents scope issues with things like for
, and let
s aren't hoisted to the top of functions so you can't declare them out of order.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is interesting, though, that const
s can't be reassigned. That might be useful (I have pretty much never needed to use the same name for a variable).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know they're not really mutable, but it's mostly intent. You should strive to have as little mutability in your code as possible and I think this helps reinforce that with warnings and convention.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really immutable, but can't be reassigned*
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/let has some good information on the differences
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Prefer const
to var
or let
"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's more "Use const
when you don't have to rebind the value, otherwise use let
and avoid var
" so I'm having a hard time making it a single rule.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't imagine that people will use let
all that much.
What about:
- Use
const
for declaring variables that will never be re-assigned, andlet
otherwise. - Avoid
var
to declare variables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a fan.
It's been a while, anyone have more thoughts on this? |
I dislike like that we're having a styleguide based on a new technology that we haven't used extensively. And it's definately not a best practice |
That's fair, but "Use CoffeeScript" isn't a best practice either. |
Whether we have a guide or not, the code we write has a style. We can certainly change things as we write more ES6, but putting a stake in the ground seems to make sense. |
Does the ES6 community have an existing styleguide that we can refer to? |
https://github.com/mozilla/addon-sdk/wiki/Coding-style-guide is the best example I could find of one. |
Fair point.
Dito, can we do something like @mike-burns suggested and refer to the community based styleguide and then "override" things in our own? |
I don't think there's a "blessed" ES6 style guide out there that people reference often. I assume the closest would be what I linked above, which is similar to what we already have here in a lot of cases. I'm hesitant to refer to it because it suggests things like not using braces for one liners in if statements. I also think it's much easier to start with our own guides telling what to do rather than reference another guide and saying what not to do from it. |
This has been sitting here for a while, any objections to merging? |
I had one tweak above for the |
* Add initial JavaScript style guide * Add ES6 to best practices via babel
40e192f
to
d175383
Compare
This is the first shot at a JavaScript style guide to get us talking about what should go in it. I've also replaced "Use CoffeeScript" from best practices with "Use ES6" (but sightly longer). A good bit of this echoes the CoffeeScript guides, but that's expected.
I also need to try and expand on
sample.js
.