[BPK-964] Upgrade eslint versions #6

shaundon · 2017-10-25T09:33:48Z

Things to discuss:

Should this be a major release as it's likely to contain 'breaking' changes, i.e. consumer builds probably won't pass due to all the rule changes.
In our monorepo we've added a lot of rule opt-outs to .eslintrc. Should those be mirrored here? Or is it left up to consumers. Maybe we could put the opt-outs into the .eslintrc of backpack-react-scripts?

matthewdavidson · 2017-10-25T09:35:39Z

@shaundon the opt outs should be removed from backpack and placed in eslint-config-skyscanner as a source of truth.

And yeah, it' defo a major release.

matthewdavidson · 2017-10-25T09:38:07Z

Also can you incorporate the removal of np that i started in #4 please? 😇 (i'll close #4 in favor of this)

shaundon · 2017-10-25T12:44:06Z

New changes:

Fixed broken test.
Updated changelog.
Changed to a major release.
Migrated rule opt outs from monorepo to this one.
Removed np dev dependency (it was np)

matthewdavidson · 2017-10-25T13:33:23Z

index.js

-    'valid-jsdoc': ['error']
+    'valid-jsdoc': ['error'],
+
+    // We've decided we don't want this for now as it makes writing JSX more painful.


I suggest "Disabled whilst incompatibilities still exist with react/jsx-closing-tag-location, react/jsx-indent & max-len."

matthewdavidson · 2017-10-25T13:35:39Z

index.js

+    // See https://github.com/airbnb/javascript/issues/1584#issuecomment-335667272
+    "function-paren-newline": 0,
+
+    // False positive on custom propTypes + isRequired. https://github.com/yannickcr/eslint-plugin-react/issues/1389


I suggest "Disabled whilst false positives still exist with custom propTypes + isRequired.".

Also can we put the "See jsx-eslint/eslint-plugin-react#1389" on a newline like the previous one.

matthewdavidson · 2017-10-25T13:35:58Z

index.js

+    // False positive on custom propTypes + isRequired. https://github.com/yannickcr/eslint-plugin-react/issues/1389
+    "react/no-typos": 0,
+
+    // See https://github.com/evcohen/eslint-plugin-jsx-a11y/issues/339


Can we explain this one a bit more? (similar to previous examples)

matthewdavidson · 2017-10-25T13:36:41Z

index.js

+    }],
+
+    // See https://github.com/evcohen/eslint-plugin-jsx-a11y/issues/282
+    // We should probably change our code to adhere to the standard here.


Again can we explain what's going on here and why "we" aren't adhering to the standard here?

Actually I think this rule should be inside the monorepo, not here.

Rather than fixing an incompatibility, it's just used to work around our own code which isn't fully adherent to this rule yet. It's probably not a great idea to make the rule this lax for every consumer as it's a generally good rule to have.

matthewdavidson · 2017-10-25T13:36:58Z

test/pass.jsx

-    this.state = {
-      key: 'value',
-    };
+    this.state = {};


this ctor looks redundant now...?

matthewdavidson · 2017-10-25T13:38:19Z

package.json

@@ -1,6 +1,6 @@
 {
  "name": "eslint-config-skyscanner",
-  "version": "2.0.0",
+  "version": "3.0.0",


How are you planning to publish this? Usually i'd use np (installed as a global npm module) to publish it. This allows you too choose which version you want and it updates the package.json for you.

Perhaps we should add publishing docs to the readme for this

Ah ok - I wondered what np was for!

matthewdavidson · 2017-10-25T13:39:25Z

changelog.md

 and this project adheres to [Semantic Versioning](http://semver.org/).

+## 3.0.0 - Upgraded `esling-config-airbnb peer dependencies`
+### Changed


We need to dramatically improve the changelog here i think. I know you copied what I had put in #4, but i'll admit that it was rubbish! Essentially we need to itemise every major difference here (think of the things we had to fix in the backpack monorepo upgrade).

I agree, but my concern is that there are so many changes, new rules etc that we'll end up duplicating their changelog inside our own repo. I think it would be clearer if we just provided links to the official changelogs, e.g.:

### Changed - Upgraded the following peer dependencies: - babel-eslint: `^7.2.3` -> `^8.0.1` - [eslint](https://github.com/eslint/eslint/blob/master/CHANGELOG.md): `^3.17.1` -> `^4.9.0` - [eslint-config-airbnb](https://github.com/airbnb/javascript/blob/master/packages/eslint-config-airbnb/CHANGELOG.md): `^14.1.0` -> `^16.1.0` - [eslint-plugin-import](https://github.com/benmosher/eslint-plugin-import/blob/master/CHANGELOG.md): `^2.2.0` -> `^2.8.0` - [eslint-plugin-jsx-a11y](https://github.com/evcohen/eslint-plugin-jsx-a11y/blob/master/CHANGELOG.md): `^4.0.0` -> `^6.0.2` - [eslint-plugin-react](https://github.com/yannickcr/eslint-plugin-react/blob/master/CHANGELOG.md): `^6.10.0` -> `^7.4.0` There are some substantial rule changes in this upgrade so you may find that your linter fails after upgrading. [eslint --fix](https://eslint.org/docs/user-guide/command-line-interface#--fix) should be able to fix the majority of these automatically. Anything else will need to be fixed manually.

What do you think?

Everything addressed.

matteo-hertel

🚀

matthewdavidson mentioned this pull request Oct 25, 2017

WIP: Upgrade airbnb peer dependencies #4

Closed

shaundon force-pushed the bpk-964-upgrade-config branch 7 times, most recently from e044fd6 to 57e6924 Compare October 25, 2017 12:21

[BPK-964] Upgrade eslint versions

e955d7e

shaundon force-pushed the bpk-964-upgrade-config branch from 57e6924 to e955d7e Compare October 25, 2017 12:30

shaundon mentioned this pull request Oct 25, 2017

[BPK-964] Upgrade eslint-config-skyscanner Skyscanner/backpack#226

Merged

matthewdavidson previously requested changes Oct 25, 2017

View reviewed changes

shaundon force-pushed the bpk-964-upgrade-config branch 2 times, most recently from a7af91f to 133e8b5 Compare October 25, 2017 15:28

[BPK-964] Fixes based on feedback

b44cfba

shaundon force-pushed the bpk-964-upgrade-config branch from 133e8b5 to b44cfba Compare October 25, 2017 15:30

matteo-hertel approved these changes Oct 27, 2017

View reviewed changes

shaundon merged commit 47a62cf into master Oct 27, 2017

shaundon deleted the bpk-964-upgrade-config branch October 27, 2017 07:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BPK-964] Upgrade eslint versions #6

[BPK-964] Upgrade eslint versions #6

shaundon commented Oct 25, 2017

matthewdavidson commented Oct 25, 2017

matthewdavidson commented Oct 25, 2017 •

edited

Loading

shaundon commented Oct 25, 2017

matthewdavidson Oct 25, 2017

matthewdavidson Oct 25, 2017

matthewdavidson Oct 25, 2017

matthewdavidson Oct 25, 2017

shaundon Oct 25, 2017

matthewdavidson Oct 25, 2017

matthewdavidson Oct 25, 2017

shaundon Oct 25, 2017

matthewdavidson Oct 25, 2017

shaundon Oct 25, 2017

matteo-hertel left a comment

[BPK-964] Upgrade eslint versions #6

[BPK-964] Upgrade eslint versions #6

Conversation

shaundon commented Oct 25, 2017

matthewdavidson commented Oct 25, 2017

matthewdavidson commented Oct 25, 2017 • edited Loading

shaundon commented Oct 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matteo-hertel left a comment

Choose a reason for hiding this comment

matthewdavidson commented Oct 25, 2017 •

edited

Loading