V3 by BPScott · Pull Request #113 · prettier/eslint-plugin-prettier

BPScott · 2018-09-27T06:02:10Z

Version 3 takes an opportunity to clear up some logic, by updating the minimum required node version (dropping support for v4, v7 and v9), eslint version (dropping support for v3 and v4) and prettier version (requiring at least v1.13.0).

Most notably this allows us to remove pragma support and tell people to rely upon prettier's native pragma support (with a side effect of no longer needing to depend upon jest-docblock), which fixes #48 and #54.

We also drop support for the "fb" option which allows us to simplify the options passed into the rule. Getting people to use a .prettierrc is now the preferred approach.

The showInvisibles and generateDifferences have been moved into a new package so stylelint-prettier and tslint-plugin-prettier don't have to have a runtime dependency on eslint-plugin-prettier as they consume those helper functions.

This updates a few dev dependencies along the way (which could have been done as v2.x but I was on a roll)

BPScott · 2018-09-27T06:23:49Z

@not-an-aardvark: While this gives an opportunity for breaking changes and tidying up cruft, what do you think to also removing the "fb" option? It seems to be a shorthand for setting some hardcoded prettier options, a relic of the days when the plugin couldn't read .prettierrc. In my opinion that config would be better placed within a .prettierrc, so that both prettier and eslint can share that configuration.

Are there any other breaking changes that you'd like to make?

not-an-aardvark

Thanks! This generally looks good, I had a few minor suggestions for other possible breaking changes.

cc @zertosh for any thoughts/objections on removing the 'fb' option (see #113 (comment) for the motivation).

not-an-aardvark · 2018-09-27T17:12:32Z

-  - "7"
  - "6"
-  - "4"
 env:


If we're dropping support for Node 4 and 7, should we also drop support for some old versions of ESLint? There's currently some logic that only runs on versions of ESLint before 3.16.0.

Sounds good to me, I originally missed that bit of interoperability code so I thought supporting old versions was effortless. Dropping support for ESLint 3.x and killing that legacy code is a good idea.

Do you think we should drop 4.x too? It looks like there's no v4 interoperability code that we could clean up by dropping it and don't know if there's any new features in v5 we'd want to take advantage of. 5.x has been out about as long as Prettier 1.13.0 which is our new minimum so our expectation of people being up to date is pretty similar for both dependencies.

For a bit of context here's some release dates:

ESLint v5.0.0 released 22 June 2018
ESLint v4.19.1 (the last in the 4.x line) released 21 March
ESLint v4.0.0 released 11 June 2017
ESLint v3.19.0 (the last in the 3.x line) released 31 March 2017
Prettier v1.13.0 (Our new miniumum required prettier) released 27 May 2018

I have no strong preferences either way. I suppose we might as well drop support for 4.x (there aren't many breaking changes in ESLint v5, so hopefully it shouldn't be too difficult for users to upgrade).

not-an-aardvark · 2018-09-27T17:19:18Z

@@ -4,9 +4,7 @@ node_js:
  - "10"
  - "9"


Should we also drop Node 9 now that it's EOL?

I was torn as ESLint v5 left node v9 in its test matrix, but I think that was because ESLint v5 was released JUST after node v9's EOL. (ESLint v5 release 2018-06-22, Node v9 was EOLed 2018-06-30).

I'm happy to drop support for it if you are.

Sounds good to me, I don't think we need to optimize for users running EOL versions of Node.

Node v4, v7 and v9 are end of life and no longer supported.

This lets us remove some conditional checks for some APIs that we now know will be present. This shall also allow us to drop pragma support as people can use the native pragma suppport in prettier (to be done in a later commit).

@Format

Prettier now provides native pragma support using @Format or @prettier notation. Use that instead of our implementation Fixes #48, Fixes #54

These were used in the release script, but that's been abstracted away now

ESLint v5.0.0 is the minimum supported version

zertosh · 2018-09-28T01:16:17Z

@vjeux, I think it's time to finally have a global .prettierrc - wdyt?

vjeux · 2018-09-28T01:16:53Z

Prettier now uses cosmiconfig and respects .prettierrc

BPScott · 2018-09-28T21:33:18Z

@zertosh: Can you define "global"? As @vjeux says, Prettier already supports a .prettierrc that can sit within a project at the same level as node_modules. and contain:

{
   "singleQuote": true,
   "trailingComma": "all",
   "bracketSpacing": false,
   "jsxBracketSameLine": true,
   "overrides": [
    {"files": "*.js", "options": {"parser": "flow"}}
  ]
}

And those options will be honoured by node_modules/.bin/prettier some-file.js or node_modules/.bin/prettier eslint some-file.js.

Thanks to cosmiconfig you can also name that file .prettierrc.js and have it require() a shared configuration you provide from elsewhere.

zertosh · 2018-09-28T22:07:41Z

@BPScott, sorry about that comment. I should've DM @vjeux directly (which I did after his comment). That was in reference to the way we run Prettier at Facebook. This change is fine, we'll just have to make some adjustments on our end.

BPScott · 2018-09-28T22:32:07Z

Gotcha :)

So to confirm: I'm good to go ahead and drop support for the "fb" option and make it so you can only pass in an object representing a prettier object (or null)?

zertosh · 2018-09-28T22:33:29Z

@BPScott, yup!

Drop support for specifying prettier options using the "fb" shorthand string. Replace "fb" with either config in your .prettierrc or the object: `{ singleQuote: true, trailingComma: 'all', bracketSpacing: false, jsxBracketSameLine: true, parser: 'flow'}` Drop support for specifying the prettier options as `null`. Replace `null` with an empty object `{}`

azz · 2018-09-29T03:59:13Z

This simplifies this module significantly, nice!

azz · 2018-09-30T08:54:29Z

One thing worth considering: removing the optional prettier config in eslintrc, and only supporting prettierrc. The reason for this is editor integrations like prettier-vscode, prettier-atom do not read eslint config files. Same with pretty-quick and potentially other tools. It can be confusing when Prettier config is in eslintrc and your editor is formatting things "incorrectly".

azz · 2018-09-30T08:55:00Z

Gah, typing on mobile, sorry.

BPScott · 2018-09-30T22:38:50Z

We should certainly update the docs to push for "we read prettierrc, you shouldn't need to add custom config in the majority of cases". (EDIT: The docs already state that this is not recommended https://github.com/prettier/eslint-plugin-prettier/blob/master/README.md#options for the reasons you point out)

However I think it is worth keeping the config override because eslint's processor integrations means that it can potentially read not-js, parse the js files out of them and then pass those js fragments to prettier, and we need a way to force only-prettier-within-eslint to parse those file using not the standard parser for that file type. We can cover the common use-cases but there's no way to have correct behaviour for everybody.

You can see that headache in action in #79

Move these two functions into a helper module 'prettier-linter-helpers'. If you used those functions then plese reference that package directly. Fixes #101

This was removed in a prior version because other linting tools ended up depending on helper functions within eslint-plugin-prettier and did not want to inadvertently install eslint as a transitive dependency. Now that those helpers have been extracted into a separate package those other linting tools should depend upon that instead of eslint-plugin-prettier.

BPScott · 2018-10-01T05:35:17Z

Yo @not-an-aardvark, I've added a few more things since your initial review. Could you check them over and confirm you're happy with them? They are: dropping support for ESLint 3/4 and extracting two of the utility functions into a separate package so stylelint-prettier and tslint-plugin-prettier don't need to depend upon eslint-plugin-prettier. Check the commit messages for more info.

I think this is it for code changes in v3. If you're happy then this this can be merged and released as v3.0.0.

not-an-aardvark

LGTM, thanks!

not-an-aardvark · 2018-10-01T06:13:19Z

Published as v3.0.0!

BPScott force-pushed the v3 branch from d875db4 to 725b82b Compare September 27, 2018 06:27

not-an-aardvark approved these changes Sep 27, 2018

View reviewed changes

BPScott added 6 commits September 27, 2018 11:45

Breaking: Drop support for node v4, v7 and v9

a9ff023

Node v4, v7 and v9 are end of life and no longer supported.

Breaking: Remove pragma support

35a5299

Prettier now provides native pragma support using @Format or @prettier notation. Use that instead of our implementation Fixes #48, Fixes #54

Chore: Rename test files to keep them sequential

ae29355

Chore: remove two unused dependencies

1bf4639

These were used in the release script, but that's been abstracted away now

Chore: Update dependencies

f5d16ba

BPScott force-pushed the v3 branch 2 times, most recently from 1b0e4d9 to 8469044 Compare September 27, 2018 18:57

Breaking: Drop support for ESLint v3 and v4

ff78a71

ESLint v5.0.0 is the minimum supported version

BPScott force-pushed the v3 branch from 8469044 to ff78a71 Compare September 27, 2018 19:09

azz closed this Sep 30, 2018

azz reopened this Sep 30, 2018

Breaking: Extract showInvisibles and generateDifferences

f5ace2d

Move these two functions into a helper module 'prettier-linter-helpers'. If you used those functions then plese reference that package directly. Fixes #101

BPScott force-pushed the v3 branch from 256ebea to f5ace2d Compare October 1, 2018 04:38

BPScott mentioned this pull request Oct 1, 2018

Consider extracting showInvisibles / generateDifferences to a new package #101

Closed

not-an-aardvark approved these changes Oct 1, 2018

View reviewed changes

not-an-aardvark merged commit d55d79c into master Oct 1, 2018

not-an-aardvark deleted the v3 branch October 1, 2018 06:11

Uh oh!

Conversation

BPScott commented Sep 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BPScott commented Sep 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

not-an-aardvark left a comment

Choose a reason for hiding this comment

Uh oh!

not-an-aardvark Sep 27, 2018

Choose a reason for hiding this comment

Uh oh!

BPScott Sep 27, 2018

Choose a reason for hiding this comment

Uh oh!

not-an-aardvark Sep 27, 2018

Choose a reason for hiding this comment

Uh oh!

not-an-aardvark Sep 27, 2018

Choose a reason for hiding this comment

Uh oh!

BPScott Sep 27, 2018

Choose a reason for hiding this comment

Uh oh!

not-an-aardvark Sep 27, 2018

Choose a reason for hiding this comment

Uh oh!

zertosh commented Sep 28, 2018

Uh oh!

vjeux commented Sep 28, 2018

Uh oh!

BPScott commented Sep 28, 2018

Uh oh!

zertosh commented Sep 28, 2018

Uh oh!

BPScott commented Sep 28, 2018

Uh oh!

zertosh commented Sep 28, 2018

Uh oh!

azz commented Sep 29, 2018

Uh oh!

azz commented Sep 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

azz commented Sep 30, 2018

Uh oh!

BPScott commented Sep 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BPScott commented Oct 1, 2018

Uh oh!

not-an-aardvark left a comment

Choose a reason for hiding this comment

Uh oh!

not-an-aardvark commented Oct 1, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

BPScott commented Sep 27, 2018 •

edited

Loading

BPScott commented Sep 27, 2018 •

edited

Loading

azz commented Sep 30, 2018 •

edited

Loading

BPScott commented Sep 30, 2018 •

edited

Loading