Add support for ESLint doc url meta, update dependencies, support TypeScript 2.7 #84

kaelig · 2018-03-13T23:59:09Z

Released under version 20.0.0.

Breaking: the version of TypeScript supported by this plugin is 2.7.x (in line with typescript-eslint-parser’s TypeScript support)
Updated dependencies that support the new ESLint documentation URL metadata. Errors from these plugins are accompanied by a link to the documentation for the broken rule.
Updated various other dependencies to their latest versions
Dependencies are now strictly versioned for tighter control over the exact rules the plugin enforces.

Package	old	new
`eslint-plugin-ava`	`^4.4.0`	`4.5.1`
`eslint-plugin-import`	`^2.8.0`	`2.9.0`
`eslint-plugin-jest`	`^21.5.0`	`21.14.1`
`eslint-plugin-lodash`	`^2.5.0`	`2.6.1`
`eslint-plugin-node`	`^5.2.1`	`6.0.1`
`eslint-plugin-prettier`	`^2.4.0`	`2.6.0`
`eslint-plugin-promise`	`^3.6.0`	`3.7.0`
`eslint-plugin-react`	`^7.5.1`	`7.7.0`
`eslint-plugin-typescript`	`^0.8.1`	`0.10.0`

Added rules:
- react/forbid-dom-props (disabled)
- react/jsx-child-element-spacing error
- react/jsx-max-depth (disabled)
- react/jsx-sort-default-props (disabled)
- react/no-this-in-sfc error
- import/group-exports (disabled)
- import/no-self-import error
- import/no-default-export (disabled)
- import/no-useless-path-segments error
- jest/prefer-expect-assertions (disabled)
- jest/valid-expect-in-promise error
- jest/valid-describe error
- jest/consistent-test-it error
- jest/no-test-prefixes error
- jest/lowercase-name error
- jest/no-jest-import error
- promise/valid-params error
- promise/no-new-statics (disabled)
- typescript/explicit-function-return-type (disabled)
- typescript/no-non-null-assertion error
Updated import/extensions due to changes in its implementation: some extensions are explicitly allowed in imports: .svg, .png, .jpg, .ico. .json is still required as well.
Chore: updated CircleCI from v1 to v2

kaelig · 2018-03-14T02:04:11Z

Request for comments

cc @lemonmade @ismail-syed @GoodForOneFare

`jest/lowercase-name`

I believe @lemonmade likes having tests that start with lowercase, so I turned on the new rule jest/lowercase-name, however it fails on this type of code:

describes should begin with lowercase

describe('ModuleConcatenationPlugin', () => {

Should I turn jest/lowercase-name back off?

`jest/valid-describe`

Fails on this code from sewing-kit:

No async describe callback

  describe('cache', async () => {
    it('always sets cacheing to true', async () => {
      expect(
        await webpackConfig(createWorkspace({env: client})),
      ).toHaveProperty('cache', true);
      expect(
        await webpackConfig(createWorkspace({env: server})),
      ).toHaveProperty('cache', true);
    });
  });

@GoodForOneFare any thoughts? Are async callbacks generally to be avoided or should we allow it?

`jest/consistent-test-it`

Fails on this code from sewing-kit:

Prefer using 'test' instead of 'it'

it('fails', () => {
  expect(true).toBe(false);
});

By default, the rule enforces test at the root of test files, and it when nested under describe.

Thoughts?

cartogram · 2018-03-14T07:13:26Z

Personally, I would prefer the jest/lowercase-name only be enforced on it and not describe (jest-community/eslint-plugin-jest#92). My guess is that would remove most of the failures.

lemonmade

This is great! Do we have to do something to have our own rules support he new doc links thing?

lemonmade · 2018-03-14T12:15:11Z

.circleci/config.yml

+         key: v1-npm-deps-{{ arch }}-{{ checksum "yarn.lock" }}
+         paths:
+           - node_modules
+      - run: yarn run check


Probably would have been better as a separate PR?

Yeah I got a bit carried away 😅

lemonmade · 2018-03-14T12:15:54Z

lib/config/rules/import.js

+      svg: 'always',
+      png: 'always',
+      jpg: 'always',
+      ico: 'always',


Also want .graphql/ .css/ .scss?

Thanks, addressed in d2f1785

lemonmade · 2018-03-14T12:16:39Z

lib/config/rules/jest.js

@@ -17,4 +17,16 @@ module.exports = {
  'jest/prefer-to-be-undefined': 'error',
  // Ensure `expect()` is called with a single argument and there is an actual expectation made.
  'jest/valid-expect': 'error',
+  // Suggest using expect.assertions() OR expect.hasAssertions()
+  'jest/valid-expect-in-promise': 'off',


I think you are missing a rule (the above us not the correct description for the rule)

Also, this rule should definitely be on, it’s a very common problem

Good catch, it was missing https://github.com/jest-community/eslint-plugin-jest/blob/master/docs/rules/prefer-expect-assertions.md

Also, this rule should definitely be on, it’s a very common problem

Turned on in 23be4b8

lemonmade · 2018-03-14T12:17:55Z

lib/config/rules/promise.js

+  // Ensures the proper number of arguments are passed to Promise functions
+  'promise/valid-params': 'error',
+  // Avoid calling new on a Promise static method
+  'promise/no-new-statics': 'off',


I see no reason to leave this off

It’s already handled by another rule (no-new)

lemonmade · 2018-03-14T12:18:24Z

lib/config/rules/react.js

@@ -49,6 +51,8 @@ module.exports = {
  'react/no-render-return-value': 'error',
  // Prevent usage of setState
  'react/no-set-state': 'off',
+  // Prevent this from being used in stateless functional components
+  'react/no-this-in-sfc': 'off',


Same here — this is a great rule

Turned on in f18da27

lemonmade · 2018-03-14T12:20:07Z

lib/config/rules/react.js

@@ -92,6 +96,8 @@ module.exports = {

  // Enforce boolean attributes notation in JSX
  'react/jsx-boolean-value': 'error',
+  // Enforce or disallow spaces inside of curly braces in JSX attributes and expressions
+  'react/jsx-child-element-spacing': 'off',


I think we’d want this rule on as well

If Prettier already takes care of this for devs, I’m not sure this needs to be turned on

I don't know if Prettier would take care of this? It's more of a clarification mechanism for developers, and in any case, Prettier is not necessarily being used when using this particular set of rules (if it does conflict with Prettier, we can turn it off just for the prettier config)

Oh you're right, I was thinking of another rule that had to do with styles and has almost the same name. Turned on in 9978763

lemonmade · 2018-03-14T12:24:27Z

All the Jest rules are good but I think we need to disable the lowercase one for now until we can turn it off for describe, as we recommend using the full component/ class name there where appropriate.

kaelig · 2018-03-14T16:47:23Z

Thank you for the feedback!

Recap of the changes:

Allow graphql, css, sass, scss, less, styl file extensions in imports
Turn rule jest/valid-expect-in-promise on
Turn rule react/no-this-in-sfc on
Turn rule react/jsx-child-element-spacing on
Turn rule jest/lowercase-name off (for now)

How do we feel about this rule https://github.com/jest-community/eslint-plugin-jest/blob/master/docs/rules/consistent-test-it.md?

This is great! Do we have to do something to have our own rules support he new doc links thing?

Yes we do. I'm opening a new branch for that.

ismail-syed · 2018-03-14T17:38:10Z

When I wrote jest/lowercase-name , I originally had it to only flag it descriptions. The maintainers suggested enforcing describe and test. jest-community/eslint-plugin-jest#56 (comment).

Glad to see there's an open PR for adding ignore options

kaelig · 2018-03-14T21:52:32Z

Just published: 20.0.0-beta.4 🚢

@lemonmade react/jsx-child-element-spacing conflicts with prettier:

Replace {'·'} with · (prettier/prettier)

Ambiguous spacing after previous element span

I agree this is a useful rule, any suggestions on how to deal with it?

Edit: found a workaround:

                <span className={styles.HeaderTextShopify}>Shopify</span>{' '}
                Polaris

lemonmade · 2018-03-15T14:51:21Z

I still might be misunderstanding the rule, but I don't think they conflict. The biggest overlap is that Prettier might sometimes change code that this rule would flag to not require the explicit space at all (example from the rule):

<div>
  Here is a
  <a>link</a>
</div>

// becomes with Prettier:
<div>Here is a  <a>link</a></div>

// which is acceptable for this rule, which deals only with newline-separate siblings I think

I confirmed that Prettier does not try to correct the case where you make explicitly no space between newline-separate children. I think we are fine to leave this on, and it just kind of works in tandem with the Prettier formatting.

kaelig · 2018-03-15T23:56:36Z

Thanks, I guess I was a bit confused because you can't add a trailing space into an element (<Foo>foo </Foo> makes ESLint and Prettier conflict) but it's so rarely a thing that I don't think it's an issue.

Let's ship it!

Add docs meta to custom rules

Rewrite rules in imperative tone

kaelig added 12 commits March 13, 2018 16:52

Update circleci from v1 to v2

def956d

Update dependencies that support the new URL metadata

f5ecd19

Add new rules coming from the dependency update

086eb09

Update changelog

20b09d2

v20.0.0-beta.1

0fee24d

Fix rule name

f21b18d

Improve eslint output

5662ed4

Use new --require flag (compilers is deprecated) for mocha

55078ee

v20.0.0-beta.2

ed54a32

Update rule to match the new implementation

5c712bf

v20.0.0-beta.3

f26e5b1

Add missing .

5270272

kaelig mentioned this pull request Mar 14, 2018

[DO NOT MERGE] Update typescript-eslint-parser for TypeScript 2.7 support #79

Closed

Mention new TypeScript version support in Changelog

d02b662

kaelig force-pushed the eslint-docs branch from 1fd2311 to d02b662 Compare March 14, 2018 01:05

kaelig requested review from lemonmade, GoodForOneFare and ismail-syed March 14, 2018 01:07

kaelig changed the title ~~Add ESLint doc url meta~~ Add ESLint doc url meta, update dependencies, support TypeScript 2.7 Mar 14, 2018

kaelig changed the title ~~Add ESLint doc url meta, update dependencies, support TypeScript 2.7~~ Add support for ESLint doc url meta, update dependencies, support TypeScript 2.7 Mar 14, 2018

lemonmade reviewed Mar 14, 2018

View reviewed changes

kaelig added 5 commits March 14, 2018 09:01

Allow graphql, css, sass, scss, less, styl file extensions in imports

d2f1785

Turn on jest/valid-expect-in-promise rule

23be4b8

Turn rule react/no-this-in-sfc on

f18da27

Turn rule react/jsx-child-element-spacing on

9978763

Turn jest/lowercase-name rule off

5c8f31e

kaelig added 3 commits March 14, 2018 14:36

Rewrite rules in imperative tone

c94a6f0

Add docs meta to custom rules

1bd52b8

v20.0.0-beta.4

1eabeb0

Apply category changes - thanks @lemonmade ❤️

5c2df96

kaelig added 2 commits March 15, 2018 16:56

Merge pull request #87 from Shopify/docs-add-meta

8e08002

Add docs meta to custom rules

Merge pull request #86 from Shopify/docs-meta

347d913

Rewrite rules in imperative tone

kaelig merged commit 73e8bcb into master Mar 15, 2018

kaelig deleted the eslint-docs branch March 15, 2018 23:57

kaelig deployed to production March 16, 2018 00:06 Active

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for ESLint doc url meta, update dependencies, support TypeScript 2.7 #84

Add support for ESLint doc url meta, update dependencies, support TypeScript 2.7 #84

kaelig commented Mar 13, 2018 •

edited

kaelig commented Mar 14, 2018

cartogram commented Mar 14, 2018 •

edited

lemonmade left a comment

lemonmade Mar 14, 2018

kaelig Mar 14, 2018

lemonmade Mar 14, 2018

kaelig Mar 14, 2018

lemonmade Mar 14, 2018

lemonmade Mar 14, 2018

kaelig Mar 14, 2018

kaelig Mar 14, 2018

lemonmade Mar 14, 2018

kaelig Mar 14, 2018 •

edited

lemonmade Mar 14, 2018

kaelig Mar 14, 2018

lemonmade Mar 14, 2018

kaelig Mar 14, 2018

lemonmade Mar 14, 2018

kaelig Mar 14, 2018

lemonmade commented Mar 14, 2018

kaelig commented Mar 14, 2018

ismail-syed commented Mar 14, 2018 •

edited

kaelig commented Mar 14, 2018 •

edited

lemonmade commented Mar 15, 2018

kaelig commented Mar 15, 2018

Add support for ESLint doc url meta, update dependencies, support TypeScript 2.7 #84

Add support for ESLint doc url meta, update dependencies, support TypeScript 2.7 #84

Conversation

kaelig commented Mar 13, 2018 • edited

kaelig commented Mar 14, 2018

Request for comments

jest/lowercase-name

jest/valid-describe

jest/consistent-test-it

cartogram commented Mar 14, 2018 • edited

lemonmade left a 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

Choose a reason for hiding this comment

kaelig Mar 14, 2018 • edited

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

lemonmade commented Mar 14, 2018

kaelig commented Mar 14, 2018

ismail-syed commented Mar 14, 2018 • edited

kaelig commented Mar 14, 2018 • edited

lemonmade commented Mar 15, 2018

kaelig commented Mar 15, 2018

kaelig commented Mar 13, 2018 •

edited

`jest/lowercase-name`

`jest/valid-describe`

`jest/consistent-test-it`

cartogram commented Mar 14, 2018 •

edited

kaelig Mar 14, 2018 •

edited

ismail-syed commented Mar 14, 2018 •

edited

kaelig commented Mar 14, 2018 •

edited