Style Guide updates #241

aaronjudd · 2017-07-26T22:48:46Z

Updates to coding style guide, and more details/examples. We've been not enforcing the current style guide, and are half way between this document and the previous version. This is an attempt to update and clarify.

- of course, we’ll have some updates to make following this!

- start a list of code review requirements

zenweasel

Just a couple of typos

zenweasel · 2017-07-30T22:00:00Z

developer/code-reviews.md

+-   jsDoc (API documentation)
+
+    -   every file should have a summary
+    -   every method should a summary


should have

zenweasel · 2017-07-30T22:03:54Z

developer/code-reviews.md

+
+-   sufficient inline commenting
+    -   explains functionality to someone NOT reading any other docs
+    -   enough detail to assistance full documentation to expand upon


this is a little confusing? I think I get what you were going for.
Maybe: Provide enough detail so that detailed docs can expand upon the topic?

Maybe add: "Provide links to source documentation where appropriate (e.g. SO Answers, API docs, etc)"

zenweasel · 2017-07-30T22:05:53Z

developer/styleguide.md

-  * local app files
+-   Double quoted strings
+-   Well spaced function
+-   160 character line length


Didn't we change the line length to 120? Do we want to add "spaces around brackets"?

zenweasel · 2017-07-30T22:08:29Z

developer/styleguide.md


-#### Folder Names
+As a convention of there are multiple files that make up a functionality, these files should be grouped within a folder. If the files are stand-alone and the name needs to represent functionality, you can use a hyphen for the seperator.


just a suggestion but maybe "if some functionality is broken down across multiple files" rather than "make up a functionality"?

zenweasel · 2017-07-30T22:15:17Z

developer/styleguide.md

+**Bad**
+
+-   Underscores and camelCased are not to be used unless expressly required;
+


Ok but don't we have a ton of existing files like this?

zenweasel · 2017-07-30T22:17:28Z

Sorry, I missed that the WIP was on there. Feel free to ignore/dismiss.

aaronjudd · 2017-07-31T16:02:00Z

@zenweasel your comments are exactly why it's WIP -> so thanks for reviewing. We do have a ton of files that break these rules -> and because the previous rules were unclear, or unenforced -> it's been sort of a 50/50 on the styles... the question is, is this newly updated guideline -> better, easier, more x-platform, etc.. then I guess we have some clean up to do.

zenweasel · 2017-08-29T02:11:27Z

developer/styleguide.md

+-   Spaces around brackets
+-   120 character line length
+-   `import` order
+    -   npm packages


I thought we had added that we import React first.

zenweasel · 2017-08-29T22:46:35Z

This is not comprehensive but some standards I would add about naming conventions that I think are pretty standard but that's certainly subjective. These are just my opinions for discussion.

Variable names should always be meaningful
Variables that contains a boolean value should use a isThisTrue style
Methods names should use a verb/noun style when possible, e.g. checkConfiguration or addToCart
Methods that return a single value should be singular, variables that return arrays should be plural
Methods whose main purpose is to return a value should use the get prefix.
Publication names should use TitleCase

spencern · 2017-08-30T16:30:53Z

developer/code-reviews.md

+
+Here are is a brief checklist of common items reviewed in every merge.
+
+-   Code style guide adhered to and **linted** (`eslint`, 0 errors, no ignores)


There are some API's where using an eslint-disable-line is necessary because the api requires underscored rather than camelcase arguments

spencern · 2017-08-30T16:39:00Z

developer/styleguide.md

-
+-   Package name should contain hyphens to make it easier to read.
+-   Underscores are not to be used unless expressly required.
+-   Folder names should not be camelCased.


Should we have a refactor party to get everything updated to hyphens instead of camelcase?

kieckhafer · 2017-08-30T16:46:00Z

developer/code-reviews.md

+-   Code style guide adhered to and **linted** (`eslint`, 0 errors, no ignores)
+-   i18n keys on all text
+-   i18n values added to appropriate `en.json`
+-   Translations are updated (LingoHub)


Can we do a quick training session to go over LIngohub? I've looked over the shoulder of aaron doing it, but I don't think I'd know what to do on my own. Maybe after the Dev meeting on Friday or Wednesday?

Should probably involve @machikoyasuda and make sure that our deployment docs get updated with any changes to our Lingohub process as well.

spencern · 2017-08-30T16:49:27Z

To add to @zenweasel's methods list:

When to use an options object instead of a list of args - More than two args, or a method where the args list might need to expand in the future.
I think we should enforce every arg having only a single type. If you need an array of strings sometimes and a string sometimes, pass the string as an array with one string.
Single letter variables are out. Even in a simple loop. It's not worth the future pain of trying to understand that code.
Add comments to every line that can't be reasoned about by itself.
If there are multiple returns, add comments explaining what causes the method to fork.
If you add "hacky" code to get something done quickly. Document extensively what's hacky about it, add a TODO to update it, and document to the best of your knowledge how it should be done better.

Something @aaronjudd has mentioned multiple times:
TODOs should make sense outside of the context of the file they are in.
Bad
TODO: fix this

Good
TODO: Make Tags publication account for multiple shops

zenweasel · 2017-08-31T01:15:58Z

I wonder if we should require/strongly advise that all methods return some sort of value. Even if it's a true on success.

zenweasel · 2017-08-31T01:18:57Z

I think requiring a object arg for more than two argument is a little too much

zenweasel · 2017-08-31T01:44:03Z

Favor ES6 string template notation ${thisVariable} is true over string concatenation whenever possible?
Use native ES6 elements over lodash whenever there is a 1-for-1 replacement (e.g. Object.assign)
(is that in there already?)
Don't use one-line if statements
Use const except when a variable is being reassigned. (does ESLint catch that already?)
When specifying a callback always use the parenthesis to indicate the argument being accepted over the bare arrow-function form so that it's clear it's not another argument to the original function.
e.g.

thisMethodTakesACallback(arg1, arg2, (result) => {
    dostuff();
});

over

thisMethodTakesACallback(arg1, arg2, result => {
    doStuff
});

machikoyasuda · 2017-09-13T16:44:26Z

To add:

Values stored for as application parameters should be cased, and in fact, as a convention, I feel that these should all be lower / camelCased.
If this is a value that will be reused as a label, then it should be using an i18n value and getting translated (and cased in i18n)

https://github.com/reactioncommerce/reaction/pull/2807/files/e98d8b356cc8054b3273a3aac422e84d34e36cce#diff-1435181aba8c909577ab5bc27cd8790a

spencern · 2017-09-14T15:51:31Z

Some more thoughts:

Always use parens

Always use parens around all function args, in all cases, even if not technically required.
less clear

arr.find(foo => foo.bar === foobar)

more clear

arr.find((foo) => foo.bar === foobar)

No multi-line shorthand arrow functions

If you have to break your arrow function into multiple lines, it's better for readability to use curly brackets and a return rather than trying to break the shorthand arrow function into multiple lines.

less clear

arr.find(
  (foo) => foo.bar === foobar
)

more clear

arr.find((foo) => {
 return foo.bar === foobar;
})

jshimko · 2017-09-14T19:40:31Z

YES @spencern. Absolutely agree. I can't stand the ambiguity from not having those parens around function args. It's a little too shorthand.

One more related thing I might add is I like the return shorthand with arrow functions (where appropriate). Meaning that you can use parens instead of curlies with an explicit return when you only have a single expression in the function body.

So...

// instead of this
arr.find((foo) => {
 return foo.bar === foobar;
})

// prefer either of these...

arr.find((foo) => foo.bar === foobar);

arr.find((foo) => (
 foo.bar === foobar;
));

One of the places I find myself using this pattern the most is with stateless React components that only return markup.

// instead of this
const MyComponent = ({ title, content }) => {
  return (
    <div>
      <h1>{title}</h1>
      <div>{content}/</div>
    </div>  
  );
}

// prefer this
const MyComponent = ({ title, content }) => (
  <div>
    <h1>{title}</h1>
    <div>{content}/</div>
  </div>  
);

Or when you're iterating over arrays in a component...

const MyThings = ({ things }) => (
  <ul>
    {things.map((thing) => <li>{thing}</li>)}
  </ul>  
);

// or

const MyThings = ({ things }) => (
  <ul>
    {things.map((thing) => (
      <li>{thing}</li>
    ))}
  </ul>  
);

spencern · 2017-09-14T22:56:04Z

@jshimko - I hadn't considered the react use case for permitting multi-line "shorthand" arrow functions. I think that could be an exception.

I totally agree that the typical shorthand arrow functions are fine, what I'm trying to create a rule against is trying too hard to keep something a "returnless" arrow function when it would be more easily read as a arrow to curlies with a return.

spencern · 2017-09-14T23:39:10Z

Another thing that might just bug me

I don't like the shorthand Collection.findOne - e.g.

Products.findOne("abc123")

Much prefer the explicit

Products.findOne({ _id: "abc123" })

zenweasel · 2017-09-15T00:43:01Z

@jshimko Had noted these before in other conversations:
I'd like to note this as an antipattern except in a few cases. I see this wrapped around almost every handler and it seems like it's potentially silencing errors that could be cause upstream.

if (this.props.handleChange) {
       this.props.handleChange("");
     }

also, more use of isRequired in prop types would be a really good practice to start enforcing.

zenweasel · 2017-09-15T02:39:18Z

I don't think we need to go overboard with eliminating lodash because they are already compact and efficient implementations and the babel plugin helps us not ship ones we don't use. But here is the beginnings of a list of ones which there are 1-to-1 ES6 replacements for so we should not be using them.

_.extend - Can be replaced with Object.assign
_.uniq - Can in most cases (but not all) be replaced using Set e.g. [...new Set(cartItems.map((item) => item.product._id))]
_.find - can be replaced with the JS native find
_.first, _.rest and _.restParam can be replaced with destructuring syntax
_.keys and _.values can be replaced by Object.keys and Object.values
_.forEach you can use this syntax to cleanly loop over an object (which was possible but messy before):

sum = 0;
lastKey = undefined;
for (const [key, value] of Object.entries(foo)) {
  sum += value;
  lastKey = key;
}

kieckhafer · 2017-09-15T02:43:25Z

Another _ / ES6 1-1: Array.isArray = _.isArray

machikoyasuda · 2017-09-19T00:20:01Z

Avoid ternary operators https://github.com/reactioncommerce/reaction/pull/2813/files/15cf436ad27311a0be57360ab5fa6833622f46fc#diff-c17e7e40537de7cd1716d7cce4d5eb15

machikoyasuda · 2017-09-19T00:20:51Z

https://github.com/reactioncommerce/reaction/pull/2813/files/15cf436ad27311a0be57360ab5fa6833622f46fc#r138798075

machikoyasuda · 2017-09-19T00:21:46Z

https://github.com/reactioncommerce/reaction/pull/2813/files/15cf436ad27311a0be57360ab5fa6833622f46fc#r139570493

not (e), not (err), but (error) is preferred style.

spencern · 2017-09-19T01:43:19Z

A few more, while we're replacing lodash with native js:

_.map => Array.prototype.map
_.reduce => Array.prototype.reduce
_.filter => Array.prototype.filter
_.each, _.forEach => Array.prototype.forEach

For pluck from underscore and older versions of lodash:
const people = [{name: "George"}, {name: "Katherine"}, {name: "Kip"}]
_.pluck(people, name) => people.map((person) => person.name)

Add details on what happens post-merge, how triage happens, and exact commands to run before you make a PR.

machikoyasuda · 2017-09-29T19:36:37Z

Hi team - I added recommendations for variable, method naming conventions and more on lodash replacements, using ()s in methods - viewable here https://docs.reactioncommerce.com/reaction-docs/style-guide-reviews/styleguide

spencern · 2017-09-29T20:33:32Z

@machikoyasuda,
not necessarily a blocker, but I think having an internal ToC at the top of the style guide would be super helpful for devs looking for something in particular.

spencern · 2017-09-29T20:41:45Z

developer/styleguide.md

- `.eslintrc` - [http://eslint.org/](https://eslint.org/)
- `.jsbeautifyrc` - [http://jsbeautifier.org/](https://jsbeautifier.org/)
- `.editorconfig` - [http://editorconfig.org/](https://editorconfig.org/)
+### Avoid camel-casing files and folders


I think that using positive language here might be more clear.
E.g. instead of Avoid camel-casing files and folders, something like Use hyphens to separate words in files and directories or File and directory names should be hyphenated

spencern · 2017-09-29T20:42:40Z

developer/styleguide.md


+## Variables


Should these two headers both be H2?

spencern · 2017-09-29T20:47:03Z

developer/styleguide.md

- File names must start with a lowercased letter and may be camel cased if joining more than one word.
- File names may contain multiple (.) characters as needed
+### Method names should:
+- Methods names should use a verb/noun style when possible, e.g. `checkConfiguration` or `addToCart`.


Not sure if I've written this down anywhere yet, but I think in cases where we might have multiple methods acting on the same resource we should consider the following style: cart/items/add, cart/items/remove -
this is probably too much change for this PR. One of the reasons for moving to a style like this is that it will naturally organize our method jsdocs.

I've used this style in the Shopify connector which we also used to experiment with jsdocs.

spencern · 2017-09-29T20:47:49Z

developer/styleguide.md


+- Avoid ternary operators and one-line `if` statements


exceptions for this in React code?

spencern · 2017-09-29T20:50:55Z

developer/styleguide.md

+- Replace `_.filter`, with  `Array.prototype.filter`
+- Replace `_.each`, `_.forEach`, with  `Array.prototype.forEach`
+- Replace `_.isArray`, with `Array.isArray`
+- Replace `_.extend` with `Object.assign`


Missing:
_.find => Array.prototype.find
_.keys => Object.keys
_.values => Object.values

also missing _.uniq

spencern · 2017-09-29T20:52:19Z

developer/styleguide.md


+Use native ES6 elements over lodash whenever there is a 1-for-1 replacement. Here are some examples of lodash methods that can be replaced with native elements:


Should probably decide if we're going to say ES6 or ES2015 and be consistent there.

Also many of these Array.prototype methods have been around since well before ES6

I think we might just want to just say "native Javascript"?

spencern · 2017-09-29T20:53:24Z

developer/styleguide.md

- Double quoted strings
- Well spaced function
- 160 character line length
+Our rules are similar to [Airbnb JavaScript Style Guide](https://github.com/airbnb/javascript) and [Meteor Code Style](https://guide.meteor.com/code-style.html), [standard template of ESLint rules](https://www.npmjs.com/package/eslint-config-airbnb), with a few custom Reaction-specific rules:


Not necessary for this PR, but we should potentially fork and change, or create our own style guide that goes through all of our ESLint rules. Pointing people to other style guides that are kinda similar doesn't feel ideal

zenweasel · 2017-10-02T03:15:18Z

developer/code-reviews.md

+Once a pull request goes through both the automated and Core team reviews, it's ready to be merged. Here are some things you may want to consider after that:
+
+- If your pull request referenced an issue, close that issue.
+- Does your new feature require new user documentation or developer documentation? Make an issue for that in [reaction-docs](https://github.com/reactioncommerce/reaction-docs/issues).


I feel like we should make a doc PR a requirement for some PR's to be merged?

* master: Update docker.md Update docker.md Update schemas.md registerSchema documentation Fix wrong import in plugin-customizing-templates-4.md Linting (#290) Update styleguide.md Style Guide updates (#241)

aaronjudd added 2 commits July 26, 2017 15:47

Updated styleguide

3959628

- of course, we’ll have some updates to make following this!

Add code review docs

e0da73c

- start a list of code review requirements

machikoyasuda added this to In Progress in Documentation Jul 28, 2017

zenweasel requested changes Jul 30, 2017

View reviewed changes

Updated review/styleguide

efe945b

aaronjudd changed the base branch from development to master August 24, 2017 18:49

spencern self-requested a review August 28, 2017 22:59

zenweasel reviewed Aug 29, 2017

View reviewed changes

Merge branch 'master' into style-guide-reviews

5c2cf71

aaronjudd changed the title ~~[WIP] Style Guide updates~~ Style Guide updates Aug 29, 2017

aaronjudd mentioned this pull request Aug 29, 2017

Apply documented code style reactioncommerce/reaction#2755

Closed

aaronjudd requested review from jshimko and a team August 30, 2017 03:49

spencern reviewed Aug 30, 2017

View reviewed changes

kieckhafer reviewed Aug 30, 2017

View reviewed changes

aaronjudd mentioned this pull request Sep 20, 2017

Some files have names incompatible with Windows reactioncommerce/reaction#2899

Closed

machikoyasuda added 2 commits September 29, 2017 11:41

Update code-review

b8c2e0f

Add details on what happens post-merge, how triage happens, and exact commands to run before you make a PR.

Update style guide with JS recommendations from team

e672d9a

Update styleguide.md

190b0d9

spencern reviewed Sep 29, 2017

View reviewed changes

zenweasel reviewed Oct 2, 2017

View reviewed changes

Update styleguide.md

345a9ed

machikoyasuda merged commit 3f210fe into master Oct 4, 2017

machikoyasuda deleted the style-guide-reviews branch October 4, 2017 23:33


		#### Folder Names
		As a convention of there are multiple files that make up a functionality, these files should be grouped within a folder. If the files are stand-alone and the name needs to represent functionality, you can use a hyphen for the seperator.

		Bad

		- Underscores and camelCased are not to be used unless expressly required;


		Here are is a brief checklist of common items reviewed in every merge.

		- Code style guide adhered to and linted (`eslint`, 0 errors, no ignores)


		Use native ES6 elements over lodash whenever there is a 1-for-1 replacement. Here are some examples of lodash methods that can be replaced with native elements:

Style Guide updates #241

Style Guide updates #241

Conversation

aaronjudd commented Jul 26, 2017 • edited

zenweasel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zenweasel commented Jul 30, 2017

aaronjudd commented Jul 31, 2017

Choose a reason for hiding this comment

zenweasel commented Aug 29, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spencern commented Aug 30, 2017

zenweasel commented Aug 31, 2017

zenweasel commented Aug 31, 2017

zenweasel commented Aug 31, 2017 • edited

machikoyasuda commented Sep 13, 2017 • edited

spencern commented Sep 14, 2017

Always use parens

No multi-line shorthand arrow functions

jshimko commented Sep 14, 2017

spencern commented Sep 14, 2017

spencern commented Sep 14, 2017 • edited by machikoyasuda

zenweasel commented Sep 15, 2017

zenweasel commented Sep 15, 2017 • edited

kieckhafer commented Sep 15, 2017

machikoyasuda commented Sep 19, 2017

machikoyasuda commented Sep 19, 2017

machikoyasuda commented Sep 19, 2017

spencern commented Sep 19, 2017 • edited

machikoyasuda commented Sep 29, 2017

spencern commented Sep 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zenweasel Oct 2, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronjudd commented Jul 26, 2017 •

edited

zenweasel commented Aug 29, 2017 •

edited

zenweasel commented Aug 31, 2017 •

edited

machikoyasuda commented Sep 13, 2017 •

edited

spencern commented Sep 14, 2017 •

edited by machikoyasuda

zenweasel commented Sep 15, 2017 •

edited

spencern commented Sep 19, 2017 •

edited

zenweasel Oct 2, 2017 •

edited