Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: ✨ Support ESLint 8 #696

Merged
merged 7 commits into from
Apr 18, 2022
Merged

Conversation

idahogurl
Copy link
Collaborator

@idahogurl idahogurl commented Jan 24, 2022

Closes #656

These are breaking changes as the ESLint API has changed with version 8.

Major change is that format is now asynchronous. This is because the executeOnText function (now lintText) https://github.com/eslint/eslint/blob/851f1f18bd1a5da32c1b645bfcb28fadc627ad9e/lib/eslint/eslint.js#L572 is asynchronous. I would have made my changes backward compatible, but there is no backward compatible method for the asynchronous change.

These are breaking changes as the ESLint API has changed with version 8
src/index.js Outdated
Comment on lines 169 to 191
if (Array.isArray(eslintConfig.globals)) {
const tempGlobals = {};
eslintConfig.globals.forEach(g => {
const [key,value] = g.split(':');
tempGlobals[key] = value;
});
eslintConfig.globals = tempGlobals;
}

eslintConfig.overrideConfig = {
rules: eslintConfig.rules,
parser: eslintConfig.parser,
globals: eslintConfig.globals,
parserOptions: eslintConfig.parserOptions,
ignorePatterns: eslintConfig.ignorePattern,
...eslintConfig.overrideConfig,
};

delete eslintConfig.rules;
delete eslintConfig.parser;
delete eslintConfig.parserOptions;
delete eslintConfig.globals;
delete eslintConfig.ignorePattern;
Copy link
Collaborator Author

@idahogurl idahogurl Jan 24, 2022

Choose a reason for hiding this comment

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

The config accepted by the ESLint API is different now. Anything that overrides the config (rules, parser, parserOptions, globals, ignorePattern, etc) needs to be in overrideConfig not the root object. To lessen the upgrade overhead, I chose to transform the arguments and not make the user change their parameters.

Also, globals must be an object now and not an array.

.eslintrc.js Outdated
@@ -10,7 +10,8 @@ const config = {
named: "never",
asyncArrow: "always"
}
]
],
"import/no-import-module-exports": "off"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated eslint-config-kentcdodds package is set to error on this rule.

@@ -0,0 +1,4 @@
module.exports = {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The eslint-config-kentcdodds update changed some rules including removing the quotes and arrow parenthesis rules.

@idahogurl idahogurl marked this pull request as ready for review February 1, 2022 03:11
@gdibble
Copy link

gdibble commented Mar 2, 2022

🙏 TY @DanielKlys7

@sheuertz
Copy link

Is there a timeframe for this release?

@bernardocs
Copy link

Bump 🙌🏽
Merging this would be wonderful! :))

@david50407
Copy link

any updates?

@bernardocs
Copy link

bernardocs commented Apr 16, 2022

Friendly ping @DanielKlys7 @idahogurl

@idahogurl
Copy link
Collaborator Author

@bernardocs I do not have write permissions to this repo so I cannot merge.

@bernardocs
Copy link

Maybe a friendly ping to some of the guys who merge other PR's ... @zimme @hamzahamidi @kylemh 👀

@zimme
Copy link
Member

zimme commented Apr 18, 2022

I haven't touched this codebase in so long I'm not really comfortable reviewing anything without having the time to get familiar with it again first and I'm sorry to say I don't really have time as I'm too busy with my startup and my family currently.

I can probably help with repo write access, for anyone that's created a few PRs and want to help maintain the package.

@idahogurl
Copy link
Collaborator Author

@zimme Completely understandable. Could I get write access?

@kylemh
Copy link
Collaborator

kylemh commented Apr 18, 2022

@zimme I'm not gonna be able to address this one until early May. I'm traveling a little too much to have time for OSS right now. I wouldn't mind another member 😁

If you can add @idahogurl , I say go for it. I don't have the ability.

@kylemh
Copy link
Collaborator

kylemh commented Apr 18, 2022

For reference, @idahogurl I'd recommend not simply merging your PR if you do get write access. For some reason, your PR didn't go through our CI.

I tried tackling this one awhile ago in #505 but have had no luck passing CI with the brief time I had available for this project.

Maybe a git commit -m "run you dumb bot" --allow-empty could get things running 👀

Rebecca Vest added 2 commits April 18, 2022 16:41
BREAKING CHANGES: The `format` function is now asynchronous.
Closes prettier#656
@kylemh
Copy link
Collaborator

kylemh commented Apr 18, 2022

ah interesting i still need to manually tell GitHub actions you're allowed to run?!

@codecov-commenter
Copy link

codecov-commenter commented Apr 18, 2022

Codecov Report

Merging #696 (bed24b6) into master (c62769e) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #696   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            2         2           
  Lines          262       281   +19     
  Branches        67        77   +10     
=========================================
+ Hits           262       281   +19     
Impacted Files Coverage Δ
src/index.js 100.00% <100.00%> (ø)
src/utils.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c62769e...bed24b6. Read the comment docs.

Copy link
Collaborator

@kylemh kylemh left a comment

Choose a reason for hiding this comment

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

nice. this will decrease the scope of that other PR

@gdibble
Copy link

gdibble commented Apr 18, 2022

pbjTime

@kylemh kylemh merged commit d8bf1e3 into prettier:master Apr 18, 2022
@gdibble
Copy link

gdibble commented Apr 18, 2022

🙏 My sincere thanks on behalf of the community and personally

@idahogurl idahogurl deleted the eslint-8-support branch April 18, 2022 23:52
@kylemh
Copy link
Collaborator

kylemh commented Apr 19, 2022

looks like the release didn't automatically occur. I'll sort this out ASAP, but if @idahogurl gets write permissions then I presume she'll fix it ASAP!

@idahogurl
Copy link
Collaborator Author

@kylemh Already have a PR up 😄 #724

@github-actions
Copy link
Contributor

🎉 This PR is included in version 14.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@zimme
Copy link
Member

zimme commented Apr 21, 2022

Don't forget to check if prettier-eslint-cli needs updating to work with the new prettier-eslint.

@georgecrawford
Copy link

I'm struggling to get this working, as I posted here: prettier/prettier-eslint-cli#427 (comment)

"eslint": "8.13.0",
"prettier": "2.6.2",
"prettier-eslint": "14.0.0",
"prettier-eslint-cli": "5.0.1",

... and yet I'm still seeing:

$ ./node_modules/.bin/prettier-eslint file.js
prettier-eslint [ERROR]: There was trouble creating the ESLint CLIEngine.
prettier-eslint-cli [ERROR]: There was an error formatting "file.js":
    TypeError: CLIEngine is not a constructor
        at getESLintCLIEngine (./node_modules/prettier-eslint-cli/node_modules/prettier-eslint/dist/utils.js:403:12)
        at getESLintConfig (./node_modules/prettier-eslint-cli/node_modules/prettier-eslint/dist/index.js:210:51)
        at format (./node_modules/prettier-eslint-cli/node_modules/prettier-eslint/dist/index.js:75:71)
        at MapSubscriber.project (./node_modules/prettier-eslint-cli/dist/format-files.js:268:54)
        at MapSubscriber._next (./node_modules/rxjs/internal/operators/map.js:49:35)
        at MapSubscriber.Subscriber.next (./node_modules/rxjs/internal/Subscriber.js:66:18)
        at AsyncSubject.Subject.next (./node_modules/rxjs/internal/Subject.js:60:25)
        at AsyncSubject.complete (./node_modules/rxjs/internal/AsyncSubject.js:53:35)
        at handler (./node_modules/rxjs/internal/observable/bindNodeCallback.js:53:33)
        at FSReqCallback.readFileAfterClose [as oncomplete] (node:internal/fs/read_file_context:68:3)
failure formatting 1 file with prettier-eslint

Is there anything obvious that I've done wrong?

@idahogurl
Copy link
Collaborator Author

@georgecrawford There was a hotfix that came out. Upgrade to 14.0.1

@georgecrawford
Copy link

Hmm. Thanks @idahogurl but I still have the exact same problem with:

"eslint": "8.13.0",
"prettier": "2.6.2",
"prettier-eslint": "14.0.1",
"prettier-eslint-cli": "5.0.1",

Looking further into it, the issue seems to be that prettier-eslint-cli is very out of date. The package.json lists the following:

"eslint": "^5.0.0",
"prettier-eslint": "^9.0.0",

I know prettier-eslint-cli is a separate project entirely, but it seems to have stalled. Are you aware of other ways in which I can run prettier-eslint on the CLI which don't use this package, and therefore don't lock us into old ESLint and prettier-eslint?

Thanks for any advice you can give!

@idahogurl
Copy link
Collaborator Author

@georgecrawford I'm going to create a PR to update the CLI.

@georgecrawford
Copy link

georgecrawford commented Apr 27, 2022 via email

@idahogurl
Copy link
Collaborator Author

@georgecrawford Appears this is going to be harder than I thought. I ran the tests after I installed the packages and all the tests are failing for me. It uses RxJs which I am not familiar with whatsoever. I'm using Node 12 not Node 8 so don't know if that affects it.

You could write a Node script that loops through files, runs the format function on each, and then overwrites the files like the CLI does. I'm not sure about performance. I think @kentcdodds used RxJs to improve performance.

You could look into using worker threads which requires Node 14+

@smartdev322
Copy link

Hello, @idahogurl
Can you give me a detailed description and the scripts?
I am just a starter of prettier.

@idahogurl
Copy link
Collaborator Author

@georgecrawford @smartdev322 prettier-eslint-cli version 6.01 is now out. https://github.com/prettier/prettier-eslint-cli/releases/tag/v6.0.0

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

Successfully merging this pull request may close these issues.

It doesn't work with eslint v8