-
Notifications
You must be signed in to change notification settings - Fork 18
Plugin Updates and React Hooks #233
Conversation
e22a16e
to
77aa75d
Compare
c8387ee
to
3a4cdcd
Compare
| eslint-plugin-react-hooks | 1.5.0 | | ||
| @typescript-eslint/eslint-plugin | 1.5.0 | | ||
| "@typescript-eslint/parser | 1.5.0 | | ||
| babel-eslint | 10.0.1 | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I am not sure any of these plugins matter, all consumers care about is rules.
graphql: require('./lib/config/graphql'), | ||
jest: require('./lib/config/jest'), | ||
jquery: require('./lib/config/jquery'), | ||
lodash: require('./lib/config/lodash'), | ||
mocha: require('./lib/config/mocha'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think, should we just go whole-hog and remove more of the ones we basically don't use anymore, like lodash
/ jquery
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't ran the code, but looking at it seems good aside from 2 small oversights. I'm excited to see the addition of react-hooks and simplification by removing rulesets that we no-longer use.
I'll echo Chis's comments about the changelog needing to be improved as it doesn't currently tell people what they need to change to get their project back in working order. The following need to be mentioned:
- The
typescript-prettier
andtypescript-react
rulesets have been removed and what people need to do fix their configs to if they're still using one of those rulesets - The
eslint-comments
ruleset has been removed and is now enabled by default as part of core - if you're usinges5
,esnext
,react
ortypescript
then you can remove the reference toeslint-comments
. - The change from
eslint-plugin-typescript
to@typescript-eslint/eslint-plugin
has resulted in rule names changing and that people will need to update any rules that they define e.g.typescript/no-var-requires
becomes@typescript-eslint/no-var-requires
plugins: ['typescript'], | ||
plugins: [ | ||
'@typescript-eslint', | ||
'eslint-comments', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the typescript
config extends from esnext
, which in turn extends from core
which adds the eslint-comments plugin (see https://github.com/Shopify/eslint-plugin-shopify/pull/233/files#diff-03d566093aa3fdf9491a9f677f0fa467R4) so I don't believe you need to specify this again here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some reason I did need to enable it again here :(
Tangentially related: if we're removing |
c44fe91
to
34a0472
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few little wording nitpicks
README.md
Outdated
|
||
```json | ||
{ | ||
"extends": [ | ||
"plugin:shopify/esnext", | ||
"plugin:shopify/lodash", | ||
"plugin:shopify/mocha" | ||
"plugin:shopify/react", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
react
extends from esnext
- so we consider react
a core config rather than an augmenting one. jest
might be a better example of an augmenting config.
CHANGELOG.md
Outdated
|
||
* `shopify/jquery-dollar-sign-reference` has been removed. | ||
|
||
* The `ava`, `mocha`, `jquery`, `lodash`, `typescript-prettier`, `typescript-react` rulesets have been removed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be worth showing what you should use instead of typescript-prettier
and typescript-react
?
Something like:
* The `ava`, `mocha`, `jquery` and `lodash` rulesets have been removed as these tools are are not commonly used at Shopify.
* The `typescript-react` and `typescript-prettier` rulesets have been removed. Replace `["plugin:shopify/typescript-react"]` with `["plugin:shopify/typescript", "plugin:shopify/react"]` and replace`["plugin:shopify/typescript-prettier"]` with `["plugin:shopify/prettier"]`
CHANGELOG.md
Outdated
| `eslint-plugin-eslint-comments` | `3.0.1` | `3.1.1` | | ||
| `eslint-plugin-babel` | `5.1.0` | `5.3.0` | | ||
| `eslint-plugin-utils` | `2.1.0` | `2.3.0` | | ||
| `eslint-plugin-prettier` | `3.0.1` | `4.1.0` | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This didn't happen. The last published version of this is still 3.0.1
Closes #230
Updates a bunch of plugins, brings in the
@typescript
packages and adds rules for react-hooks rules.