refactor(eslint): switch to new eslint api

[robinloeffel]

Rollup Plugin Name: eslint

This PR contains:

• bugfix
• feature
• refactor
• documentation
• other

Are tests included?

• yes (bugfixes and features will not be merged without tests)
• no

Breaking Changes?

• yes (breaking changes will not be merged unless absolutely necessary)
• no

If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.

Description

With version 7 of ESLint came some changes to its API. The original version of this plugin made use of the now-deprecated CLIEngine class, which lies at the heart of every ESLint integration.

The changes introduced in this PR are as follows:

• Switch from CLIEngine to new ESLint class (see below)
• Optimize and clean up the plugin's source code
• Adapt the types accordingly
• Update the documentation
• Remove newly unnecessary dependencies
• Simplify ava setup (see below)
• Re-write tests, since the old trick of writing a custom formatter can't be used anymore
• Specify minimum node version more exactly

Breaking Changes to the API

The only change to the plugin's API for the end-user is that a formatter, which is an option directly passed to ESLint, can't be a function anymore in v7.

Simpler ava tests

The previous version of this package used a neat trick where one would use a custom inline formatter function to write the results we're getting from ESLint to variables inside of the tests. Now, sadly, this isn't possible anymore, as a formatter can only be a string pointing either to an in-built formatter like stylish, an npm package or a formatter on the file system.

Without polluting the plugin with unnecessary logic for the end-user, I didn't find a way to achieve the same level of depth in regards to testing with the previous version. I played around with returning a meta prop and accessing it from within an inline plugin, but since this plugin uses the load hook and doesn't always return code—only if ESLint's autofixed something—this way doesn't really work. Switching back to the transform hook would result in the plugin suffering from the same interop bug with typescript as before, see TrySound/rollup-plugin-eslint#47.

I'm not happy with this at all, however, and would appreciate any input on how to improve this setup. If no one has an idea, I'd still like to proceed and merge, as the improvements in other aspects are quite large, and re-visit this topic at a later stage.

[shellscape]

I'm not quite sure what to make of these changes - apologies for phrasing it that way, but it's honest. In addition to removing a bunch of tests and fixtures, you've removed the ava config that allowed us to write tests using ES module syntax and ES6+ features, and downgraded the tests' syntax. You've also removed a bunch of dependencies from package.json and I can't tell why that was done.

I'd highly suggest rebasing this branch to major, and then doing only the bare minimum of changes. Also, when making changes that are this sweeping in a collaborative repo, it's important to document (or at least comment on lines in the PR) as to why deletions and changes were made. A PR description should be verbose. As it is, you've got me scratching my head and I don't know why anything in this PR was done, and I'm afraid we can't merge this.

Edit: I just realized how many commits went to master and force pushed master back to ba1fcf9d93b930853ee0e1ad6a62b2eb11db4e1f. If you need any help getting this over the finish line I'm happy to lend a hand, large org repos can be tricky.

[robinloeffel]

Yeah, this went over quite clumsily up until now. It's my first time collaborating on such a large project, which also is a monorepo using pnpm. I'll update the PRs description, as this is now containing all of the changes I've made today. And thanks for reverting master!

At the end of this I'd actually like to bump this package to v9.0.0, as it'll bring with it some changes to the API, which come from moving on from deprecated ESLint APIs to their new equivalents under the hood. Can I already change the version in the package.json or will this be done at a later stage? Also, what I wondered about from the get-go, what does the publishing process look like?

Thanks for being so understanding, man!

[shellscape]

First off, I really appreciate the effort you've put in on this changeset.

Unfortunately you're a bit off target. The changes to the README (not including the fixes like the urls to alias that snuck in) aren't great. The options in the README should be alphabetical (even if they weren't to start with, we may have missed that during migration), and generally speaking, should match the format and verbiage of the other plugins. e.g. If \true`, does something`. We're also losing a lot of coverage with removed fixtures and tests - that's not something we want to do.

What I'd recommend is taking a step back and opening a new PR based on this one, which only applies the new API, even if the tests don't pass, and then allow us all to collaborate on a resolution so we can keep the tests in place. From there, we can work on updating the README appropriately. I think on this one you fell into the age-old trap of trying to do too much in a single iteration, something we're all guilty of from time to time regardless of experience.

[shellscape]

@robinloeffel there's a ton of changes to the README that I'd like to see. would you mind if I updated your branch directly?

[shellscape]

So I think that the API change is a good one, but the one thing that'll keep this PR from being merged is the discrepancy in the testing. We went from 11 tests in master to 2 in this PR - I don't think I've ever seen a move to a new version of a thing reduce the number of tests. They typically increase in number and coverage. Coverage dropped on this branch significantly as well:

master

  11 tests passed

----------|---------|----------|---------|---------|-------------------
File      | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
----------|---------|----------|---------|---------|-------------------
All files |   91.89 |    97.06 |     100 |   91.89 |                   
 index.js |   91.89 |    97.06 |     100 |   91.89 | 14-19             
----------|---------|----------|---------|---------|-------------------

feat/easlint-ise-new-api

  2 tests passed
----------|---------|----------|---------|---------|-------------------
File      | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
----------|---------|----------|---------|---------|-------------------
All files |   83.33 |    63.64 |     100 |   81.25 |                   
 index.js |   83.33 |    63.64 |     100 |   81.25 | 28-29,51,54,57,61 
----------|---------|----------|---------|---------|-------------------

I'd like to see the test fixtures reinstated, and even if they're redundant for coverage, run against the plugin for a test. We'll absolutely have to have equal, if not better coverage, and the tests should be rock solid.

[robinloeffel]

@robinloeffel there's a ton of changes to the README that I'd like to see. would you mind if I updated your branch directly?

Yeah, feel free!

I'll get back to the testing issue asap, but it may be some days. I recently tested positive for Covid and am still recovering.

[shellscape]

😷 oh man, I'm so sorry! Please rest up and take care of yourself. Here's hoping you're on the mend quickly!

[robinloeffel]

Hey @shellscape

Sorry for my being absent, but I just couldn't manage contributing here.
Sadly, I also don't see this changing in the (near) future, as I'm now engaged in other projects which aren't really leaving me with much leftover energy in the evening.

You can go ahead and remove me from the team, if you want, as I don't see me taking an active role for a while.
I could, however, for sure, check out and review PRs.
I leave the decision up to you.

Cheers, man!

[shellscape]

Checking in and reviewing PRs would absolutely be helpful! We'll leave you on, and no worries about the lack of time, we all understand. We'll continue your work here when we're able to.

[robinloeffel]

Alright, then we'll continue like so. Thanks for being so cool about it!

[shellscape]

A fair amount of time has passed since this PR received any love, so we're going to close it for the time being. If anyone would like to take up this work, please open a new PR and we'll continue from there.