Code Style

epicExample.ts

  /*
  SRP: I watch for UTIL_OPERATION_STARTED actions aqnd emit an UTIL_OPERATION_TIMEOUT if UTIL_OPERATION_DONE not seen in time
*/
import { Observable } from 'rxjs';
import { actionGuards, actionCreators } from '../../actions';
import { SharedEpic } from './../_helpers';

const OPERATION_TIMEOUT_WAIT = 15000;

const setConversationMapper : SharedEpic = (action$, store) => {
  return action$
    .filter(actionGuards.UTIL_OPERATION_STARTED)
    .mergeMap(({ data : { operationId : startOperationId, action }}) => {
      const done$ = action$
        .filter(actionGuards.UTIL_OPERATION_DONE)
        .filter(({ data : { operationId : doneOperationId }}) => doneOperationId === startOperationId)
        .take(1);

      return Observable.timer(OPERATION_TIMEOUT_WAIT)
        .takeUntil(done$)
        .map( () => actionCreators.UTIL_OPERATION_TIMEOUT({ operationId:  startOperationId, action }) )
        .do( (action) => console.warn(`UTIL_OPERATION_DONE not seen`, action));
    });
};

export default setConversationMapper;

prettier.md

Prettier Config Options

Print Width

Default 80 Should we use 140? Prettier strives to fit the most code into every line. With the print width set to 120, prettier may produce overly compact, or otherwise undesirable code.

Tab Width

Default 2

Use Tabs

Default False

Use Semicolons

Default True

Quotes

Default double We're mostly using single, so let's just stick with it

Trailing Commas

Default none For diffing (and because we're using node 8), we should use them everywhere

Bracket Spacing

Default { foo: bar } Optional {foo: bar} Default feels more readable to me

JSX Brackets

Default

<button
  className="prettier-class"
  id="prettier-id"
  onClick={this.handleClick}
>
  Click Here
</button>

Optional

<button
  className="prettier-class"
  id="prettier-id"
  onClick={this.handleClick}>
  Click Here
</button>

I don't have an opinion here. Which one is more readable?

Arrow Function Parentheses (for parameters)

Default - Avoid - x => x Optional - Always - (x) => x

The argument for always is that diffs become more readable. I don't have an opinion.

Change which appears the most

Removing spaces before colons in types Type definitions used to look like this:

export class ConversationRole {
  uri! : string; // communicate:role:<id>
  parentVersionUri? : string;

  externalRef! : string; // smokeball:contact:<id>
  email! : string;

  display! : string;
  initials? : string;
  roleDisplay! : string;
  active! : boolean;

  conversationUri! : string; // communicate:conversation:<id>
}

Now they look like this:

export class ConversationRole {
  uri!: string; // communicate:role:<id>
  parentVersionUri?: string;

  externalRef!: string; // smokeball:contact:<id>
  email!: string;

  display!: string;
  initials?: string;
  roleDisplay!: string;
  active!: boolean;

  conversationUri!: string; // communicate:conversation:<id>
}

tslint.md

New TSLint Rules

3.5 Group your shorthand properties at the beginning of your object declaration.

Why? It’s easier to tell which properties are using the shorthand.

const anakinSkywalker = 'Anakin Skywalker';
const lukeSkywalker = 'Luke Skywalker';

// bad
const obj = {
  episodeOne: 1,
  twoJediWalkIntoACantina: 2,
  lukeSkywalker,
  episodeThree: 3,
  mayTheFourth: 4,
  anakinSkywalker,
};

// good
const obj = {
  lukeSkywalker,
  anakinSkywalker,
  episodeOne: 1,
  twoJediWalkIntoACantina: 2,
  episodeThree: 3,
  mayTheFourth: 4,
};

Makes sense in the example given by airbnb above. Maybe makes less sense when the object is defined on a single line?

return eventCreators.CONVERSATION_UPDATED_EVENT(audience, { uri : entity.getUri(), creationTimestamp, teamUri, type, updateTimestamp });

7.12 Never mutate parameters. eslint: no-param-reassign

Why? Manipulating objects passed in as parameters can cause unwanted variable side effects in the original caller.

// bad
function f1(obj) {
  obj.key = 1;
}

// good
function f2(obj) {
  const key = Object.prototype.hasOwnProperty.call(obj, 'key') ? obj.key : 1;
}

10.4 Only import from a path in one place.

eslint: no-duplicate-imports

Why? Having multiple lines that import from the same path can make code harder to maintain.

// bad
import foo from 'foo';
// … some other imports … //
import { named1, named2 } from 'foo';

// good
import foo, { named1, named2 } from 'foo';

// good
import foo, {
  named1,
  named2,
} from 'foo';

No else after return - The else here is unnecessary. Does this improve code readability?

// bad
function foo() {
  if (x) {
    return x;
  } else {
    return y;
  }
}

// bad
function cats() {
  if (x) {
    return x;
  } else if (y) {
    return y;
  }
}

// good
function foo() {
  if (x) {
    return x;
  }

  return y;
}

// good
function cats() {
  if (x) {
    return x;
  }

  if (y) {
    return y;
  }
}

I think it makes sense, but you should really change the whole thing

if (userUri) {
    return buildContext(userUri, teamUri);
} else {
    throw new Error('Missing auth token');
}

becomes

if (!userUri) {
    throw new Error('Missing auth token');
}

return buildContext(userUri, teamUri);

I also think else if is more readable than if ... if ... This is configurable in the rule.

The name of the imported module must match the name of the thing being imported. (under the category maintainability)

import Service from 'x/y/z/Service' // good

import MyCoolService from 'x/y/z/Service' // bad

Causes problems when importing from files with the same name in different places

import sharedEpics, { SharedEpicContext } from '../../../../common_data_components/redux/epics';
import sbapi, { SbapiEpicContext } from '../../sbapi/epics';

What to do here?

8.1 When you must use an anonymous function (as when passing an inline callback), use arrow function notation. eslint: prefer-arrow-callback, arrow-spacing

Why? It creates a version of the function that executes in the context of this, which is usually what you want, and is a more concise syntax. Why not? If you have a fairly complicated function, you might move that logic out into its own named function expression.

// bad
[1, 2, 3].map(function (x) {
  const y = x + 1;
  return x * y;
});

// good
[1, 2, 3].map((x) => {
  const y = x + 1;
  return x * y;
});

This one makes sense to me.

Function Name

Applies a naming convention to function names and method names. You can configure the naming convention by passing parameters. Please note, the private-method-regex does take precedence over the static-method-regex, so a private static method must match the private-method-regex. The default values are:

[ true, { 
"method-regex": "^[a-z][\w\d]+$",
"private-method-regex": "^[a-z][\w\d]+$",
"protected-method-regex": "^[a-z][\w\d]+$",
"static-method-regex": "^[A-Z_\d]+$",
"function-regex": "^[a-z][\w\d]+$"
}

Variable Name

Five arguments may be optionally provided:

"check-format": allows only lowerCamelCased or UPPER_CASED variable names "allow-leading-underscore" allows underscores at the beginning (only has an effect if “check-format” specified) "allow-trailing-underscore" allows underscores at the end. (only has an effect if “check-format” specified) "allow-pascal-case" allows PascalCase in addition to lowerCamelCase. "allow-snake-case" allows snake_case in addition to lowerCamelCase. "ban-keywords": disallows the use of certain TypeScript keywords as variable or parameter names. These are: any, Number, number, String, string, Boolean, boolean, Undefined, undefined

By default, this only allows lowerCamelCase or UPPER_CASE. We probably want to modify this to just ban-keywords.

Prefer array literal

Use array literal syntax when declaring or instantiating array types. For example, prefer the Javascript form of string[] to the TypeScript form Array. Prefer '[]' to 'new Array()'. Prefer '[4, 5]' to 'new Array(4, 5)'. Prefer '[undefined, undefined]' to 'new Array(4)'.