Upgrade Prettier to ^1.9.2 (arrowParens: always) 🎉 & update dependencies #63

kaelig · 2018-01-03T17:01:16Z

⚠️this is likely to be a breaking change

Updated dependencies to their latest versions

Updated prettier to 1.9.2, introducing a change in function parens style (set to arrowParens: 'always'):

// Before
const foo = myArray.map(foo => {});

// After
const foo = myArray.map((foo) => {});

⚠️ Upgrade path

Your project config files (package.json, .prettierrc, .eslintrc…)
may need to be updated like so:

   "singleQuote": true,
   "bracketSpacing": false,
   "trailingComma": "es5",
+  "arrowParens": "always"

Added a prettier script: yarn prettier now prettifies source files
Prettified source files using the new config

Notable upgrades

eslint-config-prettier: v2.6.0...v2.9.0
- Added: The implicit-arrow-linebreak rule
- Added: The react/jsx-one-expression-per-line rule.
- Added: The lines-around-comment rule (as a special
  rule).
- Added: The no-unexpected-multiline rule (as a special
  rule).
eslint-plugin-ava: v4.2.2...v4.4.0
- no notable rule changes
eslint-plugin-flowtype: v2.39.1...v2.41.0
- Added: flowtype/no-flow-fix-me-comments
eslint-plugin-jsx-a11y: v6.0.2...v6.0.3
- no notable rule changes
eslint-plugin-react: v7.4.0...v7.5.1
- Added: jsx-one-expression-per-line
- Added: destructuring-assignment
- Added: no-access-state-in-setstate
- Added: button-has-type
- Added: jsx-curly-brace-presence rule ([#1310][] @jackyho112)
eslint-plugin-prettier: v2.3.1...v2.4.0
- no notable rule changes
eslint-plugin-sort-class-members: v1.2.2...v1.3.0
- no notable rule changes
eslint-plugin-typescript: v0.7.0...v0.8.1
- Added: typescript/member-naming — Enforces naming conventions for class members by visibility.
- Added: typescript/no-array-constructor — Disallow generic Array constructors
typescript-eslint-parser: v9.0.0...v11.0.0
- no notable rule changes

Todo

Document all notable rule changes
Add an explicit 'off' setting for all new rules
Add a setting for a rule we missed in a previous upgrade: eslint-plugin-import@2.8.0 exports-last.

kaelig · 2018-01-03T17:04:20Z

I'm not sure how to write tests, or if it's necessary at all to write any for the arrowParens: 'always' case. @ismail-syed thoughts?

GoodForOneFare · 2018-01-03T17:37:20Z

package.json

+    "eslint-plugin-flowtype": "^2.40.1",
+    "eslint-plugin-import": "^2.8.0",
+    "eslint-plugin-jsx-a11y": "^6.0.3",
+    "eslint-plugin-lodash": "^2.5.0",


It looks like dependencies are just being bumped without updating our configs to reflect new rules / options. Are we okay with just using whatever the defaults are?

e.g.,

eslint-plugin-lodash@2.5.0 introduces import-scope.

eslint-plugin-import@2.8.0 introduces exports-last.

Thanks for asking!

eslint-plugin-lodash@2.5.0 introduces import-scope.

Looks like we already had a rule for that, maybe the yarn version was already set to 2.5.0.

// Prefer a specific import scope (e.g. lodash/map vs lodash) 'lodash/import-scope': ['error', 'method'],

As for exports-last, it's disabled by default. Should I explicitly set it to 'off'?

I believe that we explicitly set a value for all options. @lemonmade, that's correct?

Most likely, there are many more new rules + new options. I'd rather someone trawl through changelogs and produce curated PRs, instead of a shotgun update.

Yes, we try to provide explicit settings for all rules.

Most likely, there are many more new rules + new options. I'd rather someone trawl through changelogs and produce curated PRs, instead of a shotgun update.

I checked yarn.lock and it turns out we were already using the latest version of those 2 plugins. I guess the trick is to control which version is in the yarn.lock and make sure to document those upgrades too.

Let me go through the diffs and upgrade this PR.

I've updated the body of the PR with a list of all new rules and a TODO. I'll add explicit settings for all those rules as well.

@GoodForOneFare @lemonmade I've just updated the PR with new rules explicitly set to off when applicable. Can you please have another look?

kaelig · 2018-01-05T12:47:38Z

lib/config/rules/typescript.js

-  // enforces the any type is not used. (no-any from TSLint)
-  'typescript/no-explicit-any': 'off',
+  // disallow generic Array constructors
+  'typescript/no-array-constructor': 'error',


This is set to 'error' (and not turned off), to replicate this ESLint rule:

eslint-plugin-shopify/lib/config/rules/stylistic-issues.js

Lines 85 to 86 in 9c33d00

// Disallow use of the Array constructor

'no-array-constructor': 'error',

ESLint original rule: https://eslint.org/docs/rules/no-array-constructor
eslint-plugin-typescript: https://github.com/nzakas/eslint-plugin-typescript/blob/master/docs/rules/no-array-constructor.md

Can we add this to the CHANGELOG, please.

kaelig · 2018-01-08T18:17:28Z

I think I'm done with this PR - can you please give it another 👀? Merci!

GoodForOneFare

At this point, it would be better to split this into two PRs so that the Prettier upgrade can ship today.

Are most of the rules turned off because they're not useful, or to expedite the PR?

At minimum, the PR is still missing:

lines-between-class-members
multiline-comment-style
flowtype/no-mutable-array
flowtype/no-unused-expressions
promise/no-return-in-finally

GoodForOneFare · 2018-01-09T16:03:56Z

package.json

  },
  "peerDependencies": {
    "eslint": "<5 >=4.7.1"
  },
  "optionalDependencies": {
-    "prettier": "<2.0 >= 1.7.2"
+    "prettier": "<2.0 >= 1.9.2"


I dislike that this installs prettier even when consuming projects don't use it. I'd like to move it to peerDependencies.

Make sense, @ismail-syed and @lemonmade?

Good point, but is this in the scope of this PR?

I thought optional dependencies was the right thing — doesn't peer require the installation, even if unused?

I dislike that this installs prettier even when consuming projects don't use it

I agree. Optional dependencies aren't what we expected them to be.

As per yarn docs

Optional dependencies are just that: optional. If they fail to install, Yarn will still say the install process was successful.

prettier will still be installed; in the chance that it fails, yarn install will still succeed.

I suggest we change it to a peer dependency like we do with eslint.

Here's a repo that confirms the optional/peer behaviour: https://github.com/GoodForOneFare/dependency-install-types

Optionals are installed for all consumers

Peers generate warnings during yarn install, and are not installed

That sounds like something that should be discussed outside of this PR, unless I’m missing something?

kaelig · 2018-01-09T16:18:03Z

You raise good points, and I'm sorta concerned by the overhead of maintaining this growing number of rules 🧐

As far as this PR is concerned, the missing rules are unrelated (they can be added separately, no?). Should we really hold off merging it?

GoodForOneFare · 2018-01-09T18:39:48Z

If rules were added as off to expedite this PR, they should be removed, or at least flagged with TODO.

Since the last dependency bump PR serves as a good reference for the next PR, I'd rather produce a well-curated example for everyone. That said, this isn't a formally documented rule, so ¯\(ツ)/¯

I'll defer to the original assigned reviewers, and bow out of this discussion.

lemonmade

I left what I think are the right configuration options for rules we actually want to turn on. I am fine with changing those in a follow-up PR and to ship this as-is (although in future, I'd recommend only upgrading the things you actually need to avoid having this kind of creep in the first place)

lemonmade · 2018-01-10T15:14:45Z

lib/config/rules/react.js

@@ -3,10 +3,14 @@ module.exports = {

  // Enforces consistent naming for boolean props
  'react/boolean-prop-naming': 'off',
+  // Prevent usage of button elements without an explicit type attribute
+  'react/button-has-type': 'off',


lemonmade · 2018-01-10T15:15:45Z

lib/config/rules/react.js

@@ -15,6 +19,8 @@ module.exports = {
  'react/forbid-foreign-prop-types': 'error',
  // Forbid certain propTypes
  'react/forbid-prop-types': ['error', {forbid: ['any', 'array']}],
+  // Prevent using this.state within a this.setState
+  'react/no-access-state-in-setstate': 'off',


lemonmade · 2018-01-10T15:17:34Z

lib/config/rules/react.js

@@ -90,6 +96,8 @@ module.exports = {
  'react/jsx-closing-bracket-location': ['error', {location: 'tag-aligned'}],
  // Validate closing tag location in JSX
  'react/jsx-closing-tag-location': 'error',
+  // Enforce curly braces or disallow unnecessary curly braces in JSX props and/or children
+  'react/jsx-curly-brace-presence': 'off',


['error', 'never']

ismail-syed · 2018-01-10T15:51:26Z

CHANGELOG.md

+    ```diff
+       "singleQuote": true,
+       "bracketSpacing": false,
+       "trailingComma": "es5",


Our config uses trailingComma: 'all'

Ha, is that something we should also standardize? Looks like not all codebases use it like that.

Pretty sure it is consistent, our listing rules have always enforced trailing commas.

Which code bases don't use it? They should be, unless they have a good reason not too.

In the case of eslint-plugin-shopify, the repo itself uses es5 so we don't break consumers who may be on older versions of node. (eg. u2). We could add a build step in this repo to solve that, but don't think it's worth the effort.

ismail-syed

Looks good to me. Just a couple comments.

Can we append & update dependencies to the title of the PR. When this gets merged it'll be easier to read through the commit history and understand the changes.
Can we add the newly introduced rules to the changelog as well - example

ismail-syed · 2018-01-10T16:40:39Z

Did we do a beta test with this on any repos?

…o prettier-upgrade

…hopify into prettier-upgrade

kaelig · 2018-01-12T02:17:13Z

Can we append & update dependencies to the title of the PR. When this gets merged it'll be easier to read through the commit history and understand the changes.

👍

Can we add the newly introduced rules to the changelog as well

👍

Added implicit-arrow-linebreak rule
Added lines-around-comment rule (as a special
rule).
Added no-unexpected-multiline rule (as a special rule).
Added flowtype/no-flow-fix-me-comments
Added react/jsx-one-expression-per-line
Added react/destructuring-assignment
Added react/no-access-state-in-setstate
Added react/button-has-type
Added react/jsx-curly-brace-presence
Added typescript/member-naming
Added typescript/no-array-constructor

Did we do a beta test with this on any repos?

I don't think we have. What codebase would be a good candidate for a test?

ismail-syed · 2018-01-12T02:22:30Z

I don't think we have. What codebase would be a good candidate for a test?

I usually do web and sewing-kit. Sometimes admin to verify if there's no major 💥

However, it might be nice to spread out the testing codebases. Anything blocking polaris-react or polaris-styleguide from being candidates.

kaelig · 2018-01-12T02:26:52Z

Thanks! Can you do web and sewing-kit? I'll do polaris-react.

On polaris-styleguide we are so many versions of sewing-kit behind that it will definitely 💥

kaelig requested review from lemonmade and ismail-syed January 3, 2018 17:01

kaelig force-pushed the prettier-upgrade branch from 310405d to 3234e92 Compare January 3, 2018 17:01

GoodForOneFare reviewed Jan 3, 2018

View reviewed changes

kaelig commented Jan 5, 2018

View reviewed changes

kaelig added 8 commits January 8, 2018 19:07

Upgrade dependencies

72eb9b1

Set prettier config arrowParens to 'always'

7045707

Prettify source files

28bd698

Add prettier script to package.json

c0d84c5

Update changelog.md

8cad0ce

Set up new rules and explicitly turn them off when applicable

d04c552

Link to updates details in changelog

dc2952f

Upgrade eslint and eslint-plugin-flowtype

422b9d3

kaelig force-pushed the prettier-upgrade branch from e14d003 to 422b9d3 Compare January 8, 2018 18:10

Set new flowtype rule to off flowtype/no-flow-fix-me-comments

0be388b

Move new script Changelog entry under 'Added'

d2172cb

GoodForOneFare suggested changes Jan 9, 2018

View reviewed changes

GoodForOneFare reviewed Jan 9, 2018

View reviewed changes

Merge branch 'master' into prettier-upgrade

d9d4a5f

lemonmade approved these changes Jan 10, 2018

View reviewed changes

ismail-syed reviewed Jan 10, 2018

View reviewed changes

ismail-syed approved these changes Jan 10, 2018

View reviewed changes

kaelig changed the title ~~Upgrade Prettier to ^1.9.2 (arrowParens: always) 🎉~~ Upgrade Prettier to ^1.9.2 (arrowParens: always) 🎉 & update dependencies Jan 12, 2018

kaelig added 2 commits January 11, 2018 17:57

Merge branch 'master' of github.com:Shopify/eslint-plugin-shopify int…

9497035

…o prettier-upgrade

Upgrade babel-eslint to 8.2.1

2a9eb04

kaelig added 3 commits January 11, 2018 18:01

Prettify files

ce57c0a

Merge branch 'prettier-upgrade' of github.com:Shopify/eslint-plugin-s…

f3f2bab

…hopify into prettier-upgrade

Set new rules to sensible error defaults

237d042

Document all new rules in changelog

8353b87

kaelig force-pushed the prettier-upgrade branch from 2ca2d5f to 8353b87 Compare January 12, 2018 02:18

kaelig changed the base branch from master to beta January 12, 2018 02:34

kaelig force-pushed the prettier-upgrade branch from 5ade785 to 8353b87 Compare January 12, 2018 02:34

kaelig merged commit 82a072f into beta Jan 12, 2018

kaelig deleted the prettier-upgrade branch January 12, 2018 02:35

kaelig deployed to staging January 12, 2018 02:41 Active

kaelig mentioned this pull request Mar 27, 2020

Add PR template Shopify/web-configs#137

Closed

4 tasks

	// Disallow use of the Array constructor
	'no-array-constructor': 'error',

Upgrade Prettier to ^1.9.2 (arrowParens: always) 🎉 & update dependencies #63

Upgrade Prettier to ^1.9.2 (arrowParens: always) 🎉 & update dependencies #63

Conversation

kaelig commented Jan 3, 2018 • edited Loading

⚠️ Upgrade path

Notable upgrades

Todo

kaelig commented Jan 3, 2018 • edited Loading

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 Jan 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaelig commented Jan 8, 2018

GoodForOneFare 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

kaelig commented Jan 9, 2018

GoodForOneFare commented Jan 9, 2018

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

ismail-syed Jan 12, 2018 • edited Loading

Choose a reason for hiding this comment

ismail-syed left a comment • edited Loading

Choose a reason for hiding this comment

ismail-syed commented Jan 10, 2018

kaelig commented Jan 12, 2018 • edited Loading

ismail-syed commented Jan 12, 2018

kaelig commented Jan 12, 2018

kaelig commented Jan 3, 2018 •

edited

Loading

kaelig commented Jan 3, 2018 •

edited

Loading

kaelig Jan 5, 2018 •

edited

Loading

ismail-syed Jan 12, 2018 •

edited

Loading

ismail-syed left a comment •

edited

Loading

kaelig commented Jan 12, 2018 •

edited

Loading