Skip to content
This repository has been archived by the owner on Aug 18, 2020. It is now read-only.

Plugin Updates and React Hooks #233

Merged
merged 4 commits into from
Apr 8, 2019
Merged

Plugin Updates and React Hooks #233

merged 4 commits into from
Apr 8, 2019

Conversation

cartogram
Copy link
Contributor

@cartogram cartogram commented Mar 27, 2019

Closes #230

Updates a bunch of plugins, brings in the @typescript packages and adds rules for react-hooks rules.

@cartogram cartogram force-pushed the updates branch 2 times, most recently from e22a16e to 77aa75d Compare March 27, 2019 02:19
@cartogram cartogram changed the title [WIP] Plugin Updates Plugin Updates and React Hooks Mar 27, 2019
@cartogram cartogram force-pushed the updates branch 5 times, most recently from c8387ee to 3a4cdcd Compare March 28, 2019 23:22
| 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 |
Copy link
Member

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.

CHANGELOG.md Show resolved Hide resolved
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'),
Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do.

lib/config/rules/react-hooks.js Show resolved Hide resolved
Copy link
Member

@BPScott BPScott left a 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 and typescript-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 using es5, esnext, react or typescript then you can remove the reference to eslint-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

lib/config/react.js Outdated Show resolved Hide resolved
plugins: ['typescript'],
plugins: [
'@typescript-eslint',
'eslint-comments',
Copy link
Member

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

Copy link
Member

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 :(

CHANGELOG.md Outdated Show resolved Hide resolved
@BPScott
Copy link
Member

BPScott commented Apr 2, 2019

Tangentially related: if we're removing typescript-react and typescript-prettier then we'll need to update webgen

@cartogram cartogram force-pushed the updates branch 4 times, most recently from c44fe91 to 34a0472 Compare April 8, 2019 19:16
Copy link
Member

@BPScott BPScott left a 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",
Copy link
Member

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.
Copy link
Member

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` |
Copy link
Member

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add eslint-plugin-react-hooks
3 participants