Commit Graph

3 Commits

Author SHA1 Message Date
Jamie Wong
d30bb2ef7e
Fix the build for node 13.x, make travis test 10, 12, 13, stable (#263)
@JustinBeckwith pointed out in #262 that `npm install` was broken in node 13.x, and @DanielRuf pointed in #254 that test fail for node 11+ because of a change to stability of sorting.

This PR seeks to address both of those.

The installation issue was fixed by just regenerating `package-lock.json` without needing to bump any of the direct dependency versions. The test failure issue requires manual intervention.

To fix the sort stability issue, I updated the tests to use the stable sort values (these were all the correct values, though some of the test values were incorrect).

To make the suite still pass for node 10, I added a hack where I override `Array.prototype.sort` with a stable implementation that's *only* used in tests (See comments in code for a justification for why)

## Test Plan

Before this PR: `npm install` on node 13.x fails & `npm run jest` results in test failures
After this PR: `npm install` on node 13.x passes & `npm run jest` passes for node 10, 12, and 13.
2020-04-20 08:26:59 -07:00
Alan Pierce
404aacfa46 Address review comments
* Switch to `bracketSpacing: false`.
* Add prettier-ignore in one case.
2018-04-14 16:19:26 -07:00
Alan Pierce
1bcb88670b Set up Prettier and run it on the whole codebase
* Install prettier, set up the config file, and run it on all ts and tsx files.
* Install eslint and configure it with just eslint-plugin-prettier to check to
  make sure that prettier has been run.
* Add a basic .travis.yml that runs eslint.

There are other style things that might be nice to enforce with ESLint/TSLint,
like using const, import order, etc, but this commit just focuses on prettier,
which gets most of the way there.

One annoying issue for now is that typescript-eslint-parser gives a big warning
message since they haven't updated to officially support TypeScript 2.8 yet. We
aren't even using any ESLint rules that need the parser, but if we don't include
it, ESLint will crash. TS2.8 support is hopefully coming really soon, though:
https://github.com/eslint/typescript-eslint-parser/pull/454

As for the prettier config specifically, see https://prettier.io/docs/en/options.html
for the available options.

Config settings that seem non-controversial:

Semicolons: You don't use semicolons. (I prefer semicolons, but either way is fine.)

Quote style: Looks like you consistently use single quotes outside JSX and double
quotes in JSX, which is the `singleQuote: true` option.

Config settings worth discussion:

Line width: You don't have a specific max. I put 100 since I think it's a good number
for people (like both of us, probably) who find 80 a bit cramped. (At Benchling we use
110.) Prettier has a big red warning box recommending 80, but I still prefer 100ish.

Bracket spacing: This is `{foo}` vs `{ foo }` for imports, exports, object literals,
and destructuring. Looks like you're inconsistent but lean toward spaces. I personally
really dislike bracket spacing (it feels inconsistent with arrays and function calls),
but I'm certainly fine with it and Prettier has it enabled by default, so I kept it
enabled.

Trailing comma style: Options are "no trailing commas", "trailing commas for
everything exception function calls and parameter lists", and "trailing commas
everywhere". TypeScript can handle trailing commas everywhere, so there isn't a
concern with tooling. You're inconsistent, and it looks like you tend to not have
trailing commas, but I think it's probably best to just have them everywhere, so I
enabled them.

JSX Brackets: You're inconsistent about this, I think. I'd prefer to just keep the
default and wrap the `>` to the next line.

Arrow function parens: I only found two cases of arrow functions with one param
(both `c => c.frame === frame`), and both omitted the parens, so I kept the
default of omitting parens. This makes it mildly more annoying to add a typescript
type or additional param, which is a possible reason for always requiring parens.

Everything else is non-configurable, although it's possible some places would be
better with a `// prettier-ignore` comment (but I usually try to avoid those).
2018-04-14 08:40:06 -07:00