refactor(eslint)!: Prepare eslint for rollup 3 and upgrade eslint to newest version #1236

colingm · 2022-08-09T13:54:24Z

Rollup Plugin Name: `plugin-eslint`

This PR contains:

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

List any relevant issue numbers:

[@rollup/plugin-eslint] There is a conflict between @rollup/plugin-typescript and @rollup/plugin-eslint #1233
[@rollup/plugin-eslint] There is a conflict between @rollup/plugin-typescript and @rollup/plugin-eslint #1231
Update package dependencies of @rollup/plugin-eslint #1128
@rollup/plugin-eslint checks files after compiled them by @rollup/plugin-typescript, i.e. it checks not sources #1010
Eslint plugin should work on original files #796

Description

Prior to these changes, the eslint plugin was using an older
version of eslint that relied on a class called CLIEngine.
In newer versions this has now been deprecated and so upgrading
versions required also updating to using ESLint internally
instead. Ultimately this doesn't change the usage of the plugin
but some of the option names have changed for ESLint (notably
configFile needs to change to overrideConfigFile).

The main driving factor for this change is to prevent conflicts
that happen when using @rollup/plugin-typescript and this plugin
together. Without this update, eslint would run on the transpiled
code and so the error messages weren't as helpful. This update
will allow the two plugins to work well together and provide
helpful information about errors and warnings.

This work is also preparing eslint for the move to rollup 3 through
various package version updates.

BREAKING CHANGES:

options structure has changed due to moving from CLIEngine to ESlint. Review the new supported options below:
https://eslint.org/docs/latest/developer-guide/nodejs-api#-new-eslintoptions
requires Node 14

This work is also based on this abandoned pr: #663

packages/eslint/README.md

shellscape

Nice work, thanks for taking this on.

I'd also like to update the engines in package.json https://github.com/rollup/plugins/pull/1236/files#diff-83ca08b9de9168d7d59366dc40f4e5a4590a7645790563ad6c9fbb9699e053a1R18-R20 to >= 14 so we're in line with LTS.

colingm · 2022-08-09T14:20:53Z

@shellscape I will update the readme and the engine. Quick question about working with pnpm, I noticed there were a ton of changes to the pnpm-lock.yaml and I didn't want to commit some of them (things like the image below). Should I just not worry about those and commit all changes to that file?

shellscape · 2022-08-09T14:24:27Z

Good eye. Those are because we're still on the lockfile format from pnpm v6.34.0. I have it on my list to update that, but haven't gotten around to it yet. You'll want to use pnpm v6.34.0 to update the lockfile (run pnpm install from the repo root). I'd recommend checking the lockfile out from master first.

shellscape · 2022-08-09T14:33:55Z

Looks like you might need to run pnpm lint

colingm · 2022-08-09T14:36:30Z

any ideas?

shellscape · 2022-08-09T14:40:19Z

You're going down the rabbit hole now. There may be a version conflict between the version of eslint installed for the repo, and the version installed for the rollup eslint plugin. We're hoisting eslint in the monorepo to make it work correctly across all the projects. The version installed for the repo is a direct dep of https://github.com/rollup/eslint-config-rollup/blob/master/package.json#L28.

I'll update that later today and we'll try running CI here again

colingm · 2022-08-09T14:42:28Z

yeah it looks like it is because @typescript-eslint: 4.90 in plugins only works with eslint v7, with this update I would need to update @typescript-eslint to v5

shellscape · 2022-08-09T14:43:48Z

Yeah I'll get that updated. It's about time anyhow

colingm · 2022-08-09T14:50:26Z

I'm sure you will find this out soon enough but as a heads up it looks like just a straight upgrade of @typescript-eslint results in quite a few things popping up for me

shellscape · 2022-08-09T15:41:42Z

Turns out my editor has a plugin that doesn't support ESLint v8 so I'm working on fixing that now. Then I'll get our eslint config updated, update master, and then you can rebase from upstream. I'll keep you posted.

shellscape · 2022-08-09T16:18:34Z

Updating ESLint nuked my editor (Atom) ability to format on the fly, so now I'm trying to fix an abandoned prettier-atom plugin lol. Rabbit hole.

shellscape · 2022-08-09T17:05:10Z

Tried to fix the config issues in your fork, but it's going to require a master branch fix. Will keep you posted.

colingm · 2022-08-09T17:26:35Z

@shellscape seems like I have cursed you 😅 if you want I can work on updating the @typescript-eslint in my fork as well as long as you have some guidance for what you would like to see happen with errors like the member-ordering ones in my image above (maybe the answer is turn off those rules)

colingm · 2022-08-16T14:17:47Z

@shellscape checking back in, anything I can do to help out with upgrading @typescript-eslint? I was able to upgrade locally and get lint to pass (had to move some constructors)

shellscape · 2022-08-16T14:22:18Z

I ran into a major hurdle with my editor (Atom. I know, I know) and the linter plugins and ESLint v8 that I'm still trying to resolve. Planning on getting back to that soon.

shellscape · 2022-08-16T23:19:52Z

OK got everything working. Now, it gets dicey how this has to happen. Because of the hoisting of ESLint, we need to update the dependency only on the eslint rollup plugin in packages, and then update the eslint-config-rollup package in the root, then fix all of the errors (the import plugin is especially heinous as there were some breaking changes with how it views *.d.ts files without an accompanying .js file), and then push that to master. Once that's done, we need to rebase your fork+branch off of upstream/master. It's bananas but this eslint plugin is a special case. I should have some time in the next few days to make all that happen and then we can get this moving foward.

colingm · 2022-08-24T19:03:49Z

@shellscape I know you are probably quite busy so I don't want to bother you too much, but let me know if there is any step of the above process I can help with to make it smoother, would love to unblock this

shellscape · 2022-09-06T01:01:58Z

I still have this on my radar, not to worry

colingm · 2022-09-22T16:49:50Z

@shellscape anything I can do to help out?

shellscape · 2022-09-23T17:51:18Z

Thanks for checking in. I haven't lost track of this yet, not to worry.

lukastaegert · 2022-10-01T04:53:36Z

Hi! I just created a bunch of PRs to update all plugins for Rollup 3. These will be major version bumps as we explicitly drop Node < 14 compatibility. It would be nice if we could merge the ESLint PR with yours. It so happens that the repo ESLint issues should be solved now as well as I also updated the Repo-wide ESLint version in the process.

I also wanted to update all plugin dependencies but actually happened to give up on this one 😱

Suggestion:

Can you rebase on the branch repo/rollup-3-updates and merge the changes from fix(eslint): prepare for Rollup 3 #1296 into your PR?
Change the PR target to the branch repo/rollup-3-updates instead of master
Make sure to list both your breaking change and Node 14 compatibility in the BREAKING CHANGES of the commit message
Adapt the PR title to include "prepare for Rollup 3" so that I do not overlook it?

If you do that, then I will close #1296 and merge your PR instead together with the other PRs in the next days.

colingm · 2022-10-02T00:33:14Z

@lukastaegert i will go ahead and take care of all that on Monday morning EST, thank you!

Prior to these changes, the eslint plugin was using an older version of eslint that relied on a class called CLIEngine. In newer versions this has now been deprecated and so upgrading versions required also updating to using ESLint internally instead. Ultimately this doesn't change the usage of the plugin but some of the option names have changed for ESLint (notably configFile needs to change to overrideConfigFile). The main driving factor for this change is to prevent conflicts that happen when using @rollup/plugin-typescript and this plugin together. Without this update, eslint would run on the transpiled code and so the error messages weren't as helpful. This update will allow the two plugins to work well together and provide helpful information about errors and warnings. This change also includes some necessary package updates to prepare eslint for Rollup 3. BREAKING CHANGES: - options structure has changed due to moving from CLIEngine t ESlint. Review the new supported options below: https://eslint.org/docs/latest/developer-guide/nodejs-api#-new-eslintoptions - requires Node 14

colingm · 2022-10-04T12:37:02Z

@lukastaegert I have gone ahead and done all of the requested rebases and merges, let me know if everything looks alright or if there are any other requested changes.

lukastaegert

Looks great! Happy to merge this in the next days.

lukastaegert · 2022-10-04T13:48:08Z

packages/eslint/README.md

+[npm]: https://img.shields.io/npm/v/@rollup/plugin-eslint
+[npm-url]: https://www.npmjs.com/package/@rollup/plugin-eslint
+[size]: https://packagephobia.now.sh/badge?p=@rollup/plugin-eslint
+[size-url]: https://packagephobia.now.sh/result?p=@rollup/plugin-eslint


colingm · 2022-10-07T18:43:55Z

@lukastaegert it looks like this auto closed since you deleted the base branch maybe, is there something else you need me to do here?

lukastaegert · 2022-10-07T18:45:10Z

Damn it, I hoped it would just change your target branch to master as I merged the original branch into master. Can you somehow reopen the PR and point it to master instead?

Apparently, it worked for all other PRs pointing to that branch except yours, maybe this does not work for PRs from forks.

colingm · 2022-10-07T18:46:47Z

yeah, I will rebase real quick but I will probably need to open a new PR since it won't let me change base on this one

colingm · 2022-10-07T18:51:53Z

@lukastaegert #1309

colingm · 2022-10-07T18:53:27Z

Github doesn't like it when you delete target branches for open PRs, it chokes and dies

lukastaegert · 2022-10-07T19:03:33Z

Thanks so much

lukastaegert · 2022-10-07T19:15:28Z

Ok, auto-publish did not work, probably because refactor does not trigger releases. Will sort this out later.

colingm requested a review from shellscape as a code owner August 9, 2022 13:54

colingm force-pushed the use-new-eslint branch from d73da16 to 085e6e0 Compare August 9, 2022 14:02

shellscape reviewed Aug 9, 2022

View reviewed changes

packages/eslint/README.md Outdated Show resolved Hide resolved

colingm changed the title ~~Upgrading eslint to newest version~~ refactor(eslint): Upgrading eslint to newest version Aug 9, 2022

shellscape reviewed Aug 9, 2022

View reviewed changes

packages/eslint/README.md Outdated Show resolved Hide resolved

shellscape reviewed Aug 9, 2022

View reviewed changes

packages/eslint/README.md Outdated Show resolved Hide resolved

shellscape reviewed Aug 9, 2022

View reviewed changes

shellscape changed the title ~~refactor(eslint): Upgrading eslint to newest version~~ refactor(eslint)!: upgrade eslint to newest version Aug 9, 2022

lukastaegert mentioned this pull request Oct 1, 2022

fix(eslint): prepare for Rollup 3 #1296

Closed

9 tasks

colingm force-pushed the use-new-eslint branch from f34d299 to df363d8 Compare October 4, 2022 12:34

colingm changed the base branch from master to repo/rollup3-updates October 4, 2022 12:34

colingm changed the title ~~refactor(eslint)!: upgrade eslint to newest version~~ refactor(eslint)!: Prepare eslint for rollup 3 and upgrade eslint to newest version Oct 4, 2022

chore(commonjs): adapt test snapshots

f0d4c7a

lukastaegert approved these changes Oct 4, 2022

View reviewed changes

lukastaegert deleted the branch rollup:repo/rollup3-updates October 7, 2022 18:41

lukastaegert closed this Oct 7, 2022

colingm mentioned this pull request Oct 7, 2022

refactor(eslint)!: Prepare eslint for rollup 3 and upgrade eslint to newest version #1309

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(eslint)!: Prepare eslint for rollup 3 and upgrade eslint to newest version #1236

refactor(eslint)!: Prepare eslint for rollup 3 and upgrade eslint to newest version #1236

colingm commented Aug 9, 2022 •

edited

Loading

shellscape left a comment

colingm commented Aug 9, 2022

shellscape commented Aug 9, 2022

shellscape commented Aug 9, 2022

colingm commented Aug 9, 2022

shellscape commented Aug 9, 2022

colingm commented Aug 9, 2022 •

edited

Loading

shellscape commented Aug 9, 2022

colingm commented Aug 9, 2022 •

edited

Loading

shellscape commented Aug 9, 2022

shellscape commented Aug 9, 2022

shellscape commented Aug 9, 2022

colingm commented Aug 9, 2022

colingm commented Aug 16, 2022

shellscape commented Aug 16, 2022

shellscape commented Aug 16, 2022

colingm commented Aug 24, 2022

shellscape commented Sep 6, 2022

colingm commented Sep 22, 2022

shellscape commented Sep 23, 2022

lukastaegert commented Oct 1, 2022 •

edited

Loading

colingm commented Oct 2, 2022

colingm commented Oct 4, 2022

lukastaegert left a comment

lukastaegert Oct 4, 2022

colingm commented Oct 7, 2022

lukastaegert commented Oct 7, 2022 •

edited

Loading

colingm commented Oct 7, 2022

colingm commented Oct 7, 2022

colingm commented Oct 7, 2022

lukastaegert commented Oct 7, 2022

lukastaegert commented Oct 7, 2022

refactor(eslint)!: Prepare eslint for rollup 3 and upgrade eslint to newest version #1236

refactor(eslint)!: Prepare eslint for rollup 3 and upgrade eslint to newest version #1236

Conversation

colingm commented Aug 9, 2022 • edited Loading

Rollup Plugin Name: plugin-eslint

Description

shellscape left a comment

Choose a reason for hiding this comment

colingm commented Aug 9, 2022

shellscape commented Aug 9, 2022

shellscape commented Aug 9, 2022

colingm commented Aug 9, 2022

shellscape commented Aug 9, 2022

colingm commented Aug 9, 2022 • edited Loading

shellscape commented Aug 9, 2022

colingm commented Aug 9, 2022 • edited Loading

shellscape commented Aug 9, 2022

shellscape commented Aug 9, 2022

shellscape commented Aug 9, 2022

colingm commented Aug 9, 2022

colingm commented Aug 16, 2022

shellscape commented Aug 16, 2022

shellscape commented Aug 16, 2022

colingm commented Aug 24, 2022

shellscape commented Sep 6, 2022

colingm commented Sep 22, 2022

shellscape commented Sep 23, 2022

lukastaegert commented Oct 1, 2022 • edited Loading

colingm commented Oct 2, 2022

colingm commented Oct 4, 2022

lukastaegert left a comment

Choose a reason for hiding this comment

lukastaegert Oct 4, 2022

Choose a reason for hiding this comment

colingm commented Oct 7, 2022

lukastaegert commented Oct 7, 2022 • edited Loading

colingm commented Oct 7, 2022

colingm commented Oct 7, 2022

colingm commented Oct 7, 2022

lukastaegert commented Oct 7, 2022

lukastaegert commented Oct 7, 2022

colingm commented Aug 9, 2022 •

edited

Loading

Rollup Plugin Name: `plugin-eslint`

colingm commented Aug 9, 2022 •

edited

Loading

colingm commented Aug 9, 2022 •

edited

Loading

lukastaegert commented Oct 1, 2022 •

edited

Loading

lukastaegert commented Oct 7, 2022 •

edited

Loading