breaking: support ESLint 8.x

[MichaelDeBoey]

ESLint v8.0.0 is released 🎉

devDependency compatibility with ESLint 8:

• [x] eslint-plugin-json (https://github.com/azeemba/eslint-plugin-json/issues/68)
  • [x] https://github.com/azeemba/eslint-plugin-json/pull/70
  • [x] ~Release~

---

Closes #336

[ljharb]

@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]

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

[ljharb]

Rebased this on top of #338.

[codecov-commenter]

Codecov Report

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

@@             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%)	:arrow_down:
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]

@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]

@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]

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]

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]

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

[ljharb]

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.

[ljharb]

@PaperStrike thats exactly what the tests in this repo already do. The problem is that “the specifier that needs to be mocked” changed with eslint 8.

[PaperStrike]

@ljharb I managed to mock ModuleResolver on eslint 8. (https://github.com/PaperStrike/eslint-find-rules/tree/eslint-8)

1. Unfortunately then I found a bug in proxyquire, that it doesn't support require created by module.createRequire, which is used to actually load the mocked plugin in @eslint/eslintrc, see config-array-factory.js line 57 and line 1040. (Our ModuleResolver mock is for line 1021.)
2. I tried to mock module, to make its createRequire return the original require. And it works. (rule-finder.js line 14 - 21)
3. But then @eslint/eslintrc breaks when resolving test/fixtures/post-v5/eslint.yml, which also uses createRequire to specify the base path in relative-module-resolver.js line 23.

I can use a more complex module.createRequire mock to fix this (Edit: now used in https://github.com/MichaelDeBoey/eslint-find-rules/pull/11). But it may take dozens of lines, which seems not our jobs. Would you consider switching to another mock tool?

We don't need to understand eslint's internals at all if we handle module.createRequire well.

[ljharb]

I've pulled in your PR, thanks. What other mocking tool would you suggest?

[ljharb]

@PaperStrike looks like your commit removed all the mocking that was needed for eslint 6 and 7.

[PaperStrike]

@ljharb

What other mocking tool would you suggest?

Ah... I don't know, neither. I was just guessing there's some better tools.

looks like your commit removed all the mocking that was needed for eslint 6 and 7.

They don't need them. Mocking module is enough. You can see eslint@6 passes on Node.js >=12. Investigating why it fails on Node.js <= 11, though.

eslint@7 fails for the promises, not for the mocks.

[PaperStrike]

@ljharb It seems we need a createRequire polyfill like eslint@6.8.0 relative-module-resolver.js line 11 - 30 or https://www.npmjs.com/package/create-require.

[ljharb]

Sure, let's add that package.

[PaperStrike]

Yesterday when I was testing with eslint 8.1 (@eslint/eslintrc 1.0.3), mock passes until it met with

Error: Cannot read config file: /some-path/eslint-find-rules/test/fixtures/post-v5/eslint.yml Error: Function yaml.safeLoad is removed in js-yaml 4.

Today eslint 8.2 is released with @eslint/eslintrc 1.0.4, the error above has been fixed by https://github.com/eslint/eslintrc/pull/61. But now the console logs a number of :

UnhandledPromiseRejectionWarning: Error: Failed to load plugin 'plugin' declared in '--config': Cannot find module 'eslint-plugin-plugin'

~I think the mock still works, since the test is still considered passed if I modify the assertion to match the wrong one caused by the promises. Investigating how these UnhandledPromiseRejectionWarning are printed.~ see the comment below.

[PaperStrike]

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 https://github.com/thlorenz/proxyquire/issues/265. And looking for alternatives.

[PaperStrike]

Mock problems now should have been fixed in https://github.com/MichaelDeBoey/eslint-find-rules/pull/12 with a workaround.

[ljharb]

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

[PaperStrike]

@ljharb the unhandled rejection errors should have been fixed in https://github.com/MichaelDeBoey/eslint-find-rules/pull/12/commits/d66b44c2594bb17828038b0fc5aff484d90f172a 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.)

https://github.com/sarbbottam/eslint-find-rules/blob/9af66d5087fa813426d840edf0dcdeb796239ae4/src/lib/rule-finder.js#L37

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

[ljharb]

Tests are passing, which is great. I'm going to hold off merging this until I add a few other changes, including https://github.com/MichaelDeBoey/eslint-find-rules/pull/14.

[PaperStrike]

There're some failures on Node.js 4 with ESLint 3. I guess it's just the workflow host run too slow this time.

[PaperStrike]

@himynameisdave Why's the thumb down? The same commit passes the test in https://github.com/MichaelDeBoey/eslint-find-rules/runs/4133846207?check_suite_focus=true. Things like this happens from time to time in other repos I saw like core-js. You can easily tell this by take a simple look at the failure message, too.

@ljharb A simple "Re-run all jobs" should "fix" the failures I'm talking about.