Skip to content

Commit

Permalink
refactor: upgrade to Eslint 7.x & drop Node 8 support
Browse files Browse the repository at this point in the history
BREAKING CHANGE:
- Upgrade to Eslint 7.x requires Node 10.12 or higher. See [eslint 7.x migration guide](https://eslint.org/docs/user-guide/migrating-to-7.0.0)
- Drop Node 8 support
  • Loading branch information
Andreas Richter committed May 10, 2020
1 parent 38660ab commit f4373ab
Show file tree
Hide file tree
Showing 11 changed files with 2,200 additions and 1,600 deletions.
4 changes: 2 additions & 2 deletions .travis.yml
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
language: node_js
node_js:
- 8
- 10
- 12
- 14
deploy:
- provider: script
script: npx nlm release
skip_cleanup: true
'on':
branch: master
node: 12
node: 14
10 changes: 5 additions & 5 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

🎉🏅 Thanks for helping us improve this project! 🙏

This document outlines some of the practices we care about.
This document outlines some practices we care about.
If you have any questions or suggestions about the process,
feel free to [open an issue](#reporting-issues)
.
Expand Down Expand Up @@ -44,7 +44,7 @@ it might be better to follow the

**Note:** If you're planning on making substantial changes,
please [open an issue first to discuss your idea](#reporting-issues).
Otherwise you might end up investing a lot of work
Otherwise, you might end up investing a lot of work
only to discover that it conflicts with plans the maintainers might have.

The general steps for creating a pull request are:
Expand Down Expand Up @@ -117,7 +117,7 @@ different parts are required.
- **perf:** Performance optimizations.
- **test:** Fixing existing tests or adding missing ones.
Just like with **style**, if you add tests to a feature you just
introduced in the previous commit, consider keeping the tests and the
introduced in the previous commit, consider keeping the tests, and the
feature in the same commit instead.
- **chore:** Changes to the project setup and tools, dependency bumps,
and package-building house-keeping. `chore` is not a judgement on how you
Expand All @@ -129,10 +129,10 @@ different parts are required.
* `<subject>`: A [good git commit message subject][limit50].
- Keep it brief. If possible the whole first line should have at most 50
characters.
- Use imperative mood. "Create" instead of "creates" or "created".
- Use an imperative mood. "Create" instead of "creates" or "created".
- No period (".") at the end.
* `<body>`: Motivation for the change and any context required for understanding
the choices made. Just like the subject, it should use imperative mood.
the choices made. Just like the subject, it should use an imperative mood.
* `<references>`: Any URLs relevant to the PR go here. Use one line per URL and
prefix it with the kind of relationship, e.g. "Closes: " or "See: ". If you
are referencing an issue in your commit body or PR description, never use
Expand Down
42 changes: 26 additions & 16 deletions README.md
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# JavaScript at Groupon

This repository contains tools & guidelines for using JavaScript at Groupon.
It represents a best effort to capture the current practices.
It represents the best effort to capture the current practices.

## Writing NPM Packages

Expand All @@ -15,7 +15,7 @@ and library code is compiled down to whatever our targeted browsers support.

### Versioning and Publishing

We use [`nlm`](https://github.com/groupon/nlm) to manage our libaries.
We use [`nlm`](https://github.com/groupon/nlm) to manage our libraries.
This ensures that:

1. Every library has a usable `CHANGELOG.md` file as part of its git history.
Expand All @@ -24,32 +24,42 @@ This ensures that:

## Groupon JavaScript Style Guide

Fortunately there already is a great and well-documented style guide for JavaScript over at [airbnb/javascript](https://github.com/airbnb/javascript).
Fortunately there already is a great and well-documented style guide for JavaScript
over at [airbnb/javascript](https://github.com/airbnb/javascript).
It definitely is worth a read and in many ways we're staying fairly close to it.

There's some important differences that are mostly around our focus on sticking to features natively supported by node where possible
and a higher bar for rules that don't support `--fix`:
If a rule isn't clearly preventing bugs, it has to support `--fix` to be enabled.
There's some important differences that are mostly around our focus on sticking to
features natively supported by Node where possible, and a higher bar for rules that
don't support `--fix`: If a rule isn't clearly preventing bugs, it has to support
`--fix` to be enabled.

Additionally, our file naming convention is `kabab-case`. File names should be entirely in lowercase to avoid any renaming issues with file systems.
Additionally, our file naming convention is `kabab-case`. File names should be entirely
in lowercase to avoid any renaming issues with file systems.

Regarding code organization, we generally structure code by domain, not by layer (`todos/model.js` instead of `models/todo.js`).
Regarding code organization, we generally structure code by domain, not by layer
(`todos/model.js` instead of `models/todo.js`).

To ensure good automation support, we're also dropping any rules that conflict with prettier's formatting.
To ensure good automation support, we're also dropping any rules that conflict with
prettier's formatting.

### The Longer Answer

We split our rules into three categories:

* Mistakes: Things that should only appear because the developer made a mistake, 99% of the time.
* Mistakes: Things that should only appear because the developer made a mistake,
99% of the time.
If these errors aren't fixed, we wouldn't expect the program to work correctly.
This is the only category where we allow "error" even for rules that don't support `--fix`.
* Conventions: Points out patterns that we believe could lead to confusion or maintenance issues down the line.
If a rule isn't fixable, there should be a high bar for it to be enabled to not cause noise.
But even if a rule is enabled, it may at most warn.
* Opinions: Things that are arbitrary choices. Most whitespace and formatting rules fall into this category.
This is the only category where we allow "error" even for rules that don't support
`--fix`.
* Conventions: Points out patterns that we believe could lead to confusion or
maintenance issues down the line.
If a rule isn't fixable, there should be a high bar for it to be enabled to
not cause noise.
Even if a rule is enabled, it may at most warn.
* Opinions: Things that are arbitrary choices. Most whitespace and formatting rules
fall into this category.
Everything in here must support `--fix` and shouldn't require human intervention.

## Groupon CoffeeScript Style Guide

We include a single coffeelint config module - it allows globals appropriate for ES6, Mocha, and Node.JS coding.
We include a single coffeelint config module - it allows globals appropriate for ES6, Mocha, and Node coding.
File renamed without changes.
10 changes: 0 additions & 10 deletions examples/eslint/node8/for-await-of.js

This file was deleted.

1 change: 0 additions & 1 deletion examples/eslint/node8/for-await-of.js.json

This file was deleted.

3 changes: 0 additions & 3 deletions examples/eslint/node8/package.json

This file was deleted.

5 changes: 3 additions & 2 deletions lib/rules/mistakes.js
Original file line number Diff line number Diff line change
Expand Up @@ -90,8 +90,9 @@ module.exports = {
// See: https://eslint.org/docs/rules/no-unsafe-negation
'no-unsafe-negation': 'error',

// See: https://eslint.org/docs/rules/no-new-require
'no-new-require': 'error',
// See: https://github.com/mysticatea/eslint-plugin-node/blob/v11.1.0/docs/rules/no-mixed-requires.md
'node/no-new-require': 'error',

// See: https://eslint.org/docs/rules/no-new-symbol
'no-new-symbol': 'error',

Expand Down

0 comments on commit f4373ab

Please sign in to comment.