-
Notifications
You must be signed in to change notification settings - Fork 137
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
Style Guide updates #241
Merged
Merged
Style Guide updates #241
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
3959628
Updated styleguide
e0da73c
Add code review docs
efe945b
Updated review/styleguide
5c2cf71
Merge branch 'master' into style-guide-reviews
b8c2e0f
Update code-review
machikoyasuda e672d9a
Update style guide with JS recommendations from team
machikoyasuda 190b0d9
Update styleguide.md
machikoyasuda 345a9ed
Update styleguide.md
machikoyasuda File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,46 @@ | ||
# Code review process | ||
|
||
Code contributions are reviewed by the Reaction core and community contributors. All pull requests are reviewed and approved by at least one core contributor before merge. | ||
|
||
Here's what to expect when you make a pull request to Reaction: | ||
|
||
## Automated pull request checks | ||
|
||
As soon as pull requests are pushed, [BitHound](https://www.bithound.io/github/reactioncommerce/reaction), [CircleCI](https://circleci.com/gh/reactioncommerce/reaction) and [Synk](https://snyk.io/) run automated tests. All checks should pass successfully to ensure: | ||
|
||
- All linting rules pass withour errors or ignores. | ||
- Dependencies are not failing. | ||
- All tests pass. | ||
- No new security vulnerabilities are introduced. | ||
|
||
You can run these checks yourself by running these commands: | ||
- `eslint .` for linting | ||
- `reaction test` for all tests | ||
|
||
## Core team pull request review | ||
|
||
Every Monday, the Core team triages all new pull requests. The Core team reviews code quality rules including: | ||
|
||
- No new Atmosphere or Meteor dependencies are introduced. | ||
- No hard-coded copy: All copy and alerts should have i18n keys and values. | ||
- Updated LingoHub translations. | ||
- All new methods and files have jsdoc summaries, as outlined in [JSDoc guide](https://github.com/reactioncommerce/reaction-jsdoc#how-to-write-docs). | ||
- All folders, variables, method names follow naming conventions, outlined in [Reaction style guide](/developer/styleguide.md). | ||
|
||
The Core team also encourages in-line commenting. Use comments to: | ||
- Explain functionality to someone new to the code | ||
- Link to any external documentation | ||
|
||
|
||
## Getting feedback early and often | ||
|
||
Want to get feedback on your pull request before it's ready for merging? | ||
|
||
Push up the branch and add `[WIP]` for Work in Progress to the title and ask a question in GitHub. | ||
|
||
## Congrats! It's approved and merged. What's next? | ||
|
||
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). | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,119 +1,266 @@ | ||
# Style Guide | ||
# Coding style guide | ||
|
||
Our rules are similar to AirBnB or Meteor rules, [standard template of ESLint rules](https://www.npmjs.com/package/eslint-config-airbnb), although we have tweaked them a little with what is working best for us. | ||
As a community, Reaction follows guidelines for code style and naming conventions for variables, methods and filenames. The guide also includes tips on working with libraries in Reaction, like React, MongoDB, lodash and more. | ||
|
||
A couple of notable Reaction specific style rules: | ||
## On this page | ||
|
||
- Double quoted strings | ||
- Well spaced function | ||
- 160 character line length | ||
- `import` order | ||
- [General rules](#syntax-and-style-conventions) | ||
- [Naming files and folders](#naming-conventions) | ||
- [Packages](#working-with-packages) | ||
- [Variables](#variables) | ||
- [Methods](#methods) | ||
- [MongoDB](#working-with-collections) | ||
- [Native JavaScript](#using-native-javascript) | ||
- [React](#working-with-react) | ||
|
||
## Syntax and style conventions | ||
|
||
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: | ||
|
||
- Always double-quote strings | ||
- Give methods space | ||
- Add spaces around brackets | ||
- 120 character line-length | ||
- `import` in order: | ||
- React npm packages (`React`, `prop-types`, etc...) | ||
- other npm packages | ||
- meteor core packages | ||
- meteor (Atmosphere) packages | ||
- Meteor core packages | ||
- Meteor (Atmosphere) packages | ||
- local app files | ||
|
||
Review [Meteor Code Style](https://guide.meteor.com/code-style.html) for additional guidelines that are typical of Meteor projects. | ||
Other Reaction-specific rules are checked using various linting libraries. Find all the rules in the code: | ||
- [`.eslintrc`](https://github.com/reactioncommerce/reaction/blob/master/.eslintrc) - [ESLint](http://eslint.org) checks JavaScript style, including [ES2015](https://docs.meteor.com/packages/ecmascript.html#Supported-ES2015-Features), React and Babel. | ||
- [`.jsbeautifyrc`](https://github.com/reactioncommerce/reaction/blob/master/.jsbeautifyrc) - [JS Beautifier](jsbeautifier.org) automates code formatting | ||
- [`.editorconfig`](https://github.com/reactioncommerce/reaction/blob/master/.editorconfig) - [Editor Config](https://editorconfig.org/) standardizes file formatting | ||
|
||
To see the rules in action, run `eslint .` from the command line or use [ESLint code editor tools](https://eslint.org/docs/user-guide/integrations). | ||
|
||
💡 **ProTip!** If you're using [Atom](https://atom.io/), like the Core team, you can install all the necessary tools in one line: `apm install editorconfig atom-beautify linter linter-eslint linter-markdown linter-jsonlint linter-docker` | ||
|
||
## Naming conventions | ||
|
||
### Use hyphens to separate words in names | ||
|
||
File and directory names should be hyphenated. Not all operating systems are case sensitive, so avoid having two files named the same with differing case. | ||
|
||
Names of folders and files should be: | ||
- Lowercased | ||
- Alphanumeric characters | ||
- Hyphenated to join more words | ||
- Avoid `camelCase`, `undescore_casing` | ||
- Files may use multiple `.` as needed | ||
|
||
**Do** | ||
|
||
```sh | ||
/ui-grid/ | ||
/example-payment-method/ | ||
/social/ | ||
/bootstrap.rtl.js | ||
/index.js | ||
``` | ||
|
||
**Don't** | ||
|
||
```sh | ||
/reactionpackagename/ | ||
/address_book/ | ||
/addressBook/ | ||
/settingsContainer.js | ||
/addressBook.js | ||
/address_book.js | ||
``` | ||
|
||
## Editor Configuration | ||
## Working with packages | ||
|
||
In the Reaction app root, we have Reaction specific configuration files that can be used with most editors with the appropriate tools installed. | ||
### Create strong package names | ||
|
||
- `.eslintrc` - [http://eslint.org/](https://eslint.org/) | ||
- `.jsbeautifyrc` - [http://jsbeautifier.org/](https://jsbeautifier.org/) | ||
- `.editorconfig` - [http://editorconfig.org/](https://editorconfig.org/) | ||
Namespace package folders in this format: `<functionality>-<package-name>` or `<organization-name>-<package-name>` | ||
|
||
```sh | ||
/imports/plugins/custom/payments-authnet | ||
/imports/plugins/included/connectors-shopify | ||
/imports/plugins/custom/connectors-magento | ||
/reaction-paypal/ | ||
/yourorg-your-package/ | ||
``` | ||
|
||
These configurations also include additional rules supporting [ES2015](https://docs.meteor.com/packages/ecmascript.html#Supported-ES2015-Features) and `React`. | ||
### Group related files together into folders | ||
|
||
If there are multiple files that provide functionality that is broken down across multiple files, 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 separator. | ||
|
||
**Do** | ||
|
||
```sh | ||
/container/settings.js | ||
/container/settings.html | ||
/container/ <more files> | ||
``` | ||
|
||
### Atom | ||
or | ||
|
||
We've been using the Open Source [Atom](https://atom.io/) editor for some time now and can recommend it. | ||
```sh | ||
/settings-container.js | ||
/settings-container.html | ||
``` | ||
|
||
Setting up Atom is a quick command where we use the shell command `apm` to install the same [Atom linting packages](https://atom.io/users/AtomLinter) as we use for Reaction. | ||
**Don't** | ||
|
||
```sh | ||
apm install editorconfig atom-beautify linter linter-eslint linter-markdown linter-jsonlint linter-docker | ||
/container/settings-containers.js | ||
/container/settingsContainer.html | ||
settingsContainer.js | ||
``` | ||
|
||
### Lint | ||
## JavaScript style recommendations | ||
|
||
Reaction installs the `eslint` v3.x packages *eslint*, *eslint-plugin-react*, *babel-eslint* from [npm](https://www.npmjs.com/) as development dependencies. | ||
### Variables | ||
|
||
In the Reaction app root, we have a Reaction `eslint` configuration. | ||
#### Naming variables | ||
|
||
`.eslintrc` - [http://eslint.org/](https://eslint.org/) | ||
Variable names should be: | ||
|
||
For more details, see the [ESLint Getting Started Guide](http://eslint.org/docs/user-guide/getting-started). | ||
- Meaningful. Avoid single-letter variables. | ||
- Use `error`, instead of `e` or `err` | ||
- Variables that contain a boolean value should use a `isThisTrue` style. | ||
- Variables that return arrays should be plural. | ||
|
||
#### Markdown | ||
Publication names should: | ||
|
||
In our Markdown documentation, we use [remark-lint](https://github.com/wooorm/remark-lint) to ensure consistent Markdown styling. | ||
- Use TitleCase | ||
|
||
### Pull Requests | ||
#### Working with variables | ||
|
||
Pull request branches are evaluated using [BitHound](https://www.bithound.io/github/reactioncommerce/reaction) for insecure dependencies and code quality. | ||
- Use `const` except when a variable is being reassigned. | ||
|
||
- must pass a Linting check and *no errors* are accepted. | ||
- must pass a Dependency check and no priority packages are accepted. | ||
- must pass `reaction test` | ||
### Methods | ||
|
||
In many cases, documentation updates can be required as well. | ||
#### Naming methods | ||
|
||
Pull requests are submitted to a peer code review process before acceptance. | ||
- Methods names should use a verb/noun style when possible, e.g. `checkConfiguration` or `addToCart`. | ||
- Methods that return a single value should be singular. | ||
- Methods whose main purpose is to return a value should use the `get` prefix, e.g. `getShop`. | ||
- Avoid ternary operators and one-line `if` statements (except in React componenets) | ||
|
||
### File Naming Conventions | ||
#### Working with methods | ||
|
||
In general we use hyphens (-) and camelCase for folder names, and camelCase alone for file names. Underscores are not to be used for file or folder names unless expressly required. Be aware that not all operating systems are case sensitive, so it's not ok to have two files named the same with differing case. | ||
- Use parenthesis around all method arguments. | ||
|
||
#### Folder Names | ||
**Do** | ||
``` | ||
cartItems.find((item) => item.status === picked) | ||
``` | ||
|
||
**Don't** | ||
``` | ||
cartItems.find(item => item.status === picked) | ||
``` | ||
|
||
**Good** | ||
- When specifying a callback, always use the parenthesis to indicate the argument being accepted over the bare arrow-function form. This way, it's clear it's not another argument to the original function: | ||
|
||
- Folder names for packages must contain only lowercased alpha numeric characters and may be hyphenated if joining more than one word | ||
- Folder names all normal directories must start with a lowercased letter and may camel cased if joining more than one word | ||
**Do** | ||
|
||
``` | ||
thisMethodTakesACallback(arg1, arg2, (result) => { | ||
dostuff(); | ||
}); | ||
``` | ||
|
||
**Don't** | ||
``` | ||
thisMethodTakesACallback(arg1, arg2, result => { | ||
doStuff | ||
}); | ||
``` | ||
|
||
- When breaking arrow functions into multiple lines, use curly brackets and a `return`: | ||
|
||
// Packages in /imports/plugins | ||
ui-grid/ | ||
example-paymentmethod/ | ||
social/ | ||
taxes-avalara/ | ||
**Do** | ||
``` | ||
cartItems.find((item) => { | ||
return item.status === status; | ||
}) | ||
``` | ||
|
||
// All other folder names everywhere | ||
addressBook/ | ||
**Don't** | ||
``` | ||
cartItems.find( | ||
(item) => item.status === status | ||
) | ||
``` | ||
|
||
**Bad** | ||
### Working with collections | ||
|
||
- Package name should contain hyphens to make it easier to read. | ||
- Underscores are not to be used unless expressly required. | ||
Be explicit in querying: | ||
|
||
**Don't** | ||
``` | ||
Products.findOne("abc123") | ||
``` | ||
**Do** | ||
``` | ||
Products.findOne({ _id: "abc123" }) | ||
``` | ||
|
||
reactionpackagename/ | ||
address_book/ | ||
### Using native JavaScript | ||
|
||
#### File Names | ||
Use native JavaScript over lodash whenever there is a suitable replacement. | ||
|
||
**Good** | ||
Here are some examples of lodash methods that can be replaced with native elements: | ||
|
||
- 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 | ||
- Replace `_.map`, with `Array.prototype.map` | ||
- Replace `_.reduce`, with `Array.prototype.reduce` | ||
- Replace `_.filter`, with `Array.prototype.filter` | ||
- Replace `_.each`, `_.forEach`, with `Array.prototype.forEach` | ||
- Replace `_.isArray`, with `Array.isArray` | ||
- Replace `_.extend` with `Object.assign` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also missing |
||
|
||
### Working with React | ||
|
||
settingsContainer.js | ||
publishContainer.js | ||
addressBook.js | ||
bootstrap.rtl.js | ||
index.js | ||
Use the return shorthand with arrow functions in React components: | ||
|
||
// This is an exception as it's part of Meteor's naming convention | ||
addressBook.app-test.js | ||
**Don't** | ||
``` | ||
const MyComponent = ({ title, content }) => { | ||
return ( | ||
<div> | ||
<h1>{title}</h1> | ||
<div>{content}/</div> | ||
</div> | ||
); | ||
} | ||
``` | ||
|
||
**Bad** | ||
**Do** | ||
``` | ||
const MyComponent = ({ title, content }) => ( | ||
<div> | ||
<h1>{title}</h1> | ||
<div>{content}/</div> | ||
</div> | ||
); | ||
``` | ||
|
||
- Hyphens and underscores are not to be used unless expressly required; such is the case with Meteor for `*.app-test.js` files. Or for migration files to make the filename more readable. | ||
When iterating over arrays in a component: | ||
|
||
**Do** | ||
|
||
settings_container.js | ||
publish-container.js | ||
address_book.js | ||
``` | ||
const MyThings = ({ things }) => ( | ||
<ul> | ||
{things.map((thing) => <li>{thing}</li>)} | ||
</ul> | ||
); | ||
|
||
// or | ||
|
||
const MyThings = ({ things }) => ( | ||
<ul> | ||
{things.map((thing) => ( | ||
<li>{thing}</li> | ||
))} | ||
</ul> | ||
); | ||
``` |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 feel like we should make a doc PR a requirement for some PR's to be merged?