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

breaking: support ESLint 8.x #337

Merged
merged 6 commits into from
Nov 8, 2021
Merged

Conversation

MichaelDeBoey
Copy link
Contributor

@MichaelDeBoey MichaelDeBoey commented Aug 29, 2021

.travis.yml Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@ljharb
Copy link
Collaborator

ljharb commented Oct 9, 2021

@sarbbottam tests don't seem to be running. is that something you could look into on travis? otherwise i'll have to put up a PR switching us over to github actions.

@ljharb
Copy link
Collaborator

ljharb commented Oct 10, 2021

Filed #339 which unblocks #338, which unblocks this one.

@ljharb
Copy link
Collaborator

ljharb commented Oct 11, 2021

Rebased this on top of #338.

@ljharb ljharb marked this pull request as draft October 11, 2021 03:17
@codecov-commenter
Copy link

codecov-commenter commented Oct 11, 2021

Codecov Report

Merging #337 (e95f4ad) into master (6b4b5a0) will decrease coverage by 1.54%.
The diff coverage is 88.88%.

Impacted file tree graph

@@             Coverage Diff             @@
##            master     #337      +/-   ##
===========================================
- Coverage   100.00%   98.45%   -1.55%     
===========================================
  Files            9        9              
  Lines          181      194      +13     
===========================================
+ Hits           181      191      +10     
- Misses           0        3       +3     
Impacted Files Coverage Δ
src/lib/rule-finder.js 95.89% <87.50%> (-4.11%) ⬇️
src/bin/diff.js 100.00% <100.00%> (ø)
src/bin/find.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 6b4b5a0...e95f4ad. Read the comment docs.

@ljharb
Copy link
Collaborator

ljharb commented Oct 11, 2021

@MichaelDeBoey looks like there's a legit test failure - Error [ERR_PACKAGE_PATH_NOT_EXPORTED]: Package subpath './lib/shared/relative-module-resolver' is not defined by "exports" in /home/runner/work/eslint-find-rules/eslint-find-rules/node_modules/@eslint/eslintrc/package.json in test/lib/rule-finder.js:8:20. Presumably require('@eslint/eslintrc/lib/shared/relative-module-resolver') no longer works in v1 of that package, and we need a different try/catch/require in the test file.

@MichaelDeBoey
Copy link
Contributor Author

@ljharb Fixed the import, together with the fix of the CLIEngine & linter deprecation.

Still some failing tests, but can't figure out how to fix them.
Do you have any pointers into the right direction?

@ljharb ljharb mentioned this pull request Oct 11, 2021
22 tasks
Copy link
Collaborator

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

hmm, now there seems to be even more failing tests than before, just not because of the require issue :-/

src/lib/rule-finder.js Outdated Show resolved Hide resolved
@ljharb
Copy link
Collaborator

ljharb commented Oct 11, 2021

I think since you changed the eslintrc import to not be a deep import, the proxyquire mocks are no longer working.

If there's no deep import way to get to it, then we'll have to mock out the entire eslintrc module.

@himynameisdave
Copy link

Dying for this to drop! Just tried building + running this branch against my config, but it wasn't working (eslint.CLIEngine is not a constructor). Happy to test this more though if it helps get this over the finish line!

@MichaelDeBoey
Copy link
Contributor Author

@ljharb I've mocked @eslint/eslintrc, but tests still seem to fail.
Any idea how I can fix them?

@ljharb
Copy link
Collaborator

ljharb commented Oct 30, 2021

No, I’ve been playing with it locally too with no success. I have some changes to push up but something is failing to mock getRules properly.

@PaperStrike
Copy link
Contributor

PaperStrike commented Nov 6, 2021

Oh.. I see.

My createRequire mock isn't actually work with the calls to the require created by it. What I have done was just a working resolve method mock. So.. although it simplified the logic on eslint 3 - 7 (@eslint/eslintrc < 1), it doesn't work with @eslint/eslintrc >= 1, too. 😬 (@eslint/eslintrc >= 1 make calls to require created by createRequire.)

Will be much better if we can access the proxyquire-mocked require, or proxyquire itself supports createRequire.

filled thlorenz/proxyquire#265. And looking for alternatives.

@PaperStrike
Copy link
Contributor

Mock problems now should have been fixed in MichaelDeBoey#12 with a workaround.

@ljharb
Copy link
Collaborator

ljharb commented Nov 6, 2021

hm, so for the promise stuff, i'm not seeing those unhandled rejection warnings locally (altho i can easily reproduce the failures)

@PaperStrike
Copy link
Contributor

PaperStrike commented Nov 6, 2021

@ljharb the unhandled rejection errors should have been fixed in MichaelDeBoey@d66b44c with a workaround. You won't see them if you are using node.js > 14, too. (Don't know why. Just took a look at the checks. Whatever, it's solved.)

.map(filePath => cliEngine.isPathIgnored(filePath) ? false : cliEngine.getConfigForFile(filePath))

The promises make this line never always hit the false. Thus the failures.

ljharb and others added 2 commits November 7, 2021 15:01
Co-authored-by: Michaël De Boey <[email protected]>
Co-authored-by: Jordan Harband <[email protected]>
Co-authored-by: 华 <[email protected]>

chore: simplify process.exit mock
@ljharb ljharb changed the title feat: support ESLint 8.x breaking: support ESLint 8.x Nov 7, 2021
@ljharb
Copy link
Collaborator

ljharb commented Nov 7, 2021

Tests are passing, which is great. I'm going to hold off merging this until I add a few other changes, including MichaelDeBoey#14.

@PaperStrike

This comment has been minimized.

@ljharb ljharb force-pushed the eslint-8 branch 2 times, most recently from 87a1468 to 103025e Compare November 8, 2021 06:51
Copy link
Collaborator

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Unfortunately this is a breaking change, so I'm bundling in dropping eslint < 7.

@PaperStrike

This comment has been minimized.

@ljharb ljharb marked this pull request as ready for review November 8, 2021 07:19
@ljharb ljharb merged commit e95f4ad into sarbbottam:master Nov 8, 2021
@MichaelDeBoey MichaelDeBoey deleted the eslint-8 branch November 8, 2021 09:45
ljharb pushed a commit to MichaelDeBoey/eslint-find-rules that referenced this pull request Nov 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support ESLint 8.x
6 participants