Contributor Guide

This document collects various tips, conventions, and procedures that may be useful to Popcode contributors. In particular, any code review feedback that invokes a general principle should be accompanied by a section in this document (which will be expanded as demanded by that rule).

Code style

Most code style conventions are enforced by an extensive ESLint configuration, but there are a handful that can't be expressed as ESLint rules.

Multiline lists of things

If a list of things (array elements, object entries, function arguments/parameters, etc.) does not fit on a single line, project style is to put each element on its own line, and also put the start and end delimiters on their own line:

const toppings = [
  'cheese',
  'mushrooms',
  'olives',
  'green peppers',
  'roasted red peppers',
  'sun-dried tomatoes',
];

const meals = {
  breakfast: 'Dunkin donuts breakfast sandwich',
  lunch: 'Falafel sandwich',
  dinner: 'Burrito',
};

const dietRegime = {
  breakfast: [
    'Fruit Smoothie',
    'Egg',
  ],
  lunch: [
    'Whole Wheat Turkey Sandwich',
    'Low-fat Greek Yogurt',
    'Apple',
  ],
  dinner: 'Salad',
};

function findSnack(
  hungerLevel,
  dietaryRestrictions,
  crunchiness,
  saltiness,
  sweetness,
) {
  // implementation left to the reader
}

Destructuring should reduce repetition without reducing clarity

Destructuring can make code much more readable, but there are plenty of situations in which it has the opposite effect. Generally, use destructuring when it reduces repetition in code, and avoid it when it makes the code less clear.

For example, this code:

const {current: editor} = editorRef;

can be written much more clearly, and with no added repetition, as:

const editor = editorRef.current;

On the other hand, this code:

const type = action.type;
const payload = action.payload;

can use destructuring to remove repetition of the tokens type, action, and payload:

const {type, payload} = action;

A more complicated destructuring example:

const type = action.type;
const credentials = action.payload.credentials;

can be rewritten as:

const {type, payload: {credentials}} = action;

Special note, in this destructuring scenario, typeof payload === "undefined"

Destructuring function parameters is nearly always a good idea.

Managing dependencies

Avoiding unnecessary duplicate versions

yarn’s dependency resolution algorithm often results in multiple versions of the same package being installed, even when there is one version that satisfies all constraints. The yarn-deduplicate tool is designed to mitigate this by operating on an existing yarn.lock file, merging together duplicate dependencies wherever possible. To run it in Popcode, run this:

$ yarn deduplicate

Popcode’s CI includes a check for duplicate dependencies, and will fail if yarn deduplicate needs to be run.

Avoiding unnecessary version changes in yarn.lock

If your pull request adds, updates, or removes package dependencies, it’s important that the changes to the yarn.lock file reflect the differences in package.json, and nothing more. Particularly in the case of a merge conflict with master, yarn will sometimes inadvertently upgrade unrelated package dependencies when auto-resolving the conflict.

The easiest way to ensure that the changes to yarn.lock are kept to the necessary minimum is to do this:

$ git checkout master -- yarn.lock
$ yarn install
$ yarn deduplicate

If you’re dealing with a merge conflict in the middle of a rebase, do this instead:

$ git checkout HEAD -- yarn.lock
$ yarn install
$ yarn deduplicate
$ git add yarn.lock
$ git rebase --continue

Testing

All new tests should be written using Jest. We are actively porting old Tape/Karma tests to Jest (see Deprecations below).

Unit testing

Jest is designed for unit testing, which is to say testing the behavior of a single unit of code (typically a function or class). Mocking external dependencies of the code under test is encouraged.

Ideally, our test strategy would also include an integration test suite that runs in the browser, using something like Selenium. At this time there are no concrete plans to build an integration test suite, although efforts toward that goal are certainly welcome.

Test data

Tests often need input data that has a specific shape, mimicking the data structures produced by parts of the code other than that under test. For instance, a reducer might expect a Firebase user object in the payload of an action; clearly, when testing the reducer, that object must be produced some other way than making an actual network call using the Firebase SDK.

It’s important to avoid simply hand-coding object literals into our tests to solve this problem; this causes the knowledge of the data shape to leak all over the test suite, making it quite cumbersome to update the tests to account for changes in the behavior of the real code that generates the data in question. Secondarily, it makes tests difficult to read, since so much space is given over to large object literals.

There are a couple of preferred solutions to the problem, depending on the situation:

• If it is possible to generate test data using our own application code, this is preferred. For instance, in order to generate a test Redux state for a logic test, we can just invoke one or more actions on the root reducer to generate the state we have in mind.
• For data that normally comes from external libraries, we create factories, which centralize knowledge about how to create test objects with the right structure, containing sensible sentinel values, but also easily overridden in tests when a specific property value is needed.

Deprecations

Certain patterns in our code are deprecated, meaning that they should not be used in new modules, and they will eventually be refactored to use the currently preferred pattern in all existing cases. Specifically, look out for:

Reducers with switch

Writing reducers using a switch statement is deprecated, although most reducers currently are written this way. For new reducers, prefer handleActions as demonstrated in the console reducer.

Sagas

Our use of redux-saga has been deprecated in favor of redux-logic, which provides analogous functionality to sagas but allows us to write asynchronous code using standard async/await structures. New asynchronous/side-effect/business logic code should be written in a logic, not a saga.

Logics live in src/logic and each logic should be its own module.

Tape/Karma tests

Our tape/karma test suite is deprecated in favor of Jest unit tests. Jest tests can be found in __test__ directories throughout the codebase.

Both the deprecated tape/karma suite and the Jest suite are run as part of the CI build. It’s fine to modify existing tape tests to reflect changes in existing functionality, but new tests should be written in Jest.