diff --git a/developer/code-reviews.md b/developer/code-reviews.md new file mode 100644 index 000000000..c4dfd1e46 --- /dev/null +++ b/developer/code-reviews.md @@ -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). diff --git a/developer/styleguide.md b/developer/styleguide.md index efa3a5402..d7acac37c 100644 --- a/developer/styleguide.md +++ b/developer/styleguide.md @@ -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: `-` or `-` + +```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/ +``` -### 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` +### 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 ( +
+

{title}

+
{content}/
+
+ ); +} +``` -**Bad** +**Do** +``` +const MyComponent = ({ title, content }) => ( +
+

{title}

+
{content}/
+
+); +``` -- 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 }) => ( +
    + {things.map((thing) =>
  • {thing}
  • )} +
+); + +// or + +const MyThings = ({ things }) => ( +
    + {things.map((thing) => ( +
  • {thing}
  • + ))} +
+); +```