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

Migrate to ESLint 9 and flat configs #20

Merged
merged 6 commits into from
Oct 10, 2024
Merged

Migrate to ESLint 9 and flat configs #20

merged 6 commits into from
Oct 10, 2024

Conversation

philippfromme
Copy link
Contributor

@philippfromme philippfromme commented Oct 2, 2024

Proposed Changes

  • Migrates this plugin to ESLint 9, ESM, and flat configs.
  • Adds tests for when there are no errors and when there are errors (verifying that the configs actually work).
  • Improves test organization

Closes #19

Checklist

To ensure you provided everything we need to look at your PR:

  • Brief textual description of the changes present
  • Visual demo attached
  • Steps to try out present, i.e. using the @bpmn-io/sr tool
  • Related issue linked via Closes {LINK_TO_ISSUE} or Related to {LINK_TO_ISSUE}

@philippfromme philippfromme force-pushed the eslint-9 branch 10 times, most recently from 08e2bc5 to 9e31960 Compare October 7, 2024 11:12
@philippfromme philippfromme changed the title feat: migrate to flat configs Migrate to ESLint 9 and flat configs Oct 7, 2024
@philippfromme philippfromme marked this pull request as ready for review October 8, 2024 06:49
configs/mocha.js Show resolved Hide resolved
Copy link
Member

@nikku nikku left a comment

Choose a reason for hiding this comment

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

Could you explain how the "errors" / "no errors" is supposed to work? I see that you commented out all errors. Would such a test fail if the line did not yield the particular error?

"scripts": {
"test": "eslint ."
"test": "npm-run-all test:*",
"test:browser": "eslint browser --config browser/eslint.config.js --report-unused-disable-directives",
Copy link
Member

Choose a reason for hiding this comment

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

Smart.

@barmac
Copy link
Member

barmac commented Oct 8, 2024

Could you explain how the "errors" / "no errors" is supposed to work? I see that you commented out all errors. Would such a test fail if the line did not yield the particular error?

As far as I understand, the errors specs contain overrides which are reported if eslint does not use them, cf. #20 (review)

So if I have a snippet below, the script will report unused directive:

const life = 42; // eslint-disable-line no-unused-vars
console.log(life);

@nikku
Copy link
Member

nikku commented Oct 8, 2024

I spent quite some time trying to dig how eslint@9 handles overrides / globals. My goal was to write a test case that would pass with expect being a global, but fail if we don't declare it.

I propose that we sync (knowledge exchange) on the topic later this week @philippfromme. Seems to be of inherent complexity 🙈

@nikku
Copy link
Member

nikku commented Oct 9, 2024

Ok, I went on to drastically simplify this PR. From what it looks like 9be702a is a requirement to properly (later on) be able to have simple, per branch configurations.

@barmac @philippfromme Please review.

@nikku
Copy link
Member

nikku commented Oct 9, 2024

What I'd be interested in is whether the "full ES6" style of configuring things has a negative impact in different places (i.e. when consuming in Desktop Modeler).

➡️ I guess not, because we can provide the configuration in an mjs file, as you propose.

configs/jsx.js Show resolved Hide resolved
@barmac

This comment was marked as outdated.

@barmac
Copy link
Member

barmac commented Oct 10, 2024

LGTM 🚀

package.json Show resolved Hide resolved
.github/workflows/CI.yml Outdated Show resolved Hide resolved
@barmac
Copy link
Member

barmac commented Oct 10, 2024

One more thing [required]

@barmac
Copy link
Member

barmac commented Oct 10, 2024

OK done via decea27. Check out the comment if you want to know details.

I'd say we can squash it now.

  Closes #19

  Related to bpmn-io/internal-docs#1042

  BREAKING CHANGE: require eslint@9 (drop support for <= eslint@8)
  BREAKING CHANGE: use from ESM code-base, this module is ESM only
  BREAKING CHANGE: consume as ES module

feat!: work off react:recommended and mocha:recommended rules
Allows us to get rid of some hacks.
@nikku
Copy link
Member

nikku commented Oct 10, 2024

All cleaned up and ready to merge 🚀

@nikku nikku merged commit 55178df into main Oct 10, 2024
20 checks passed
@nikku nikku deleted the eslint-9 branch October 10, 2024 09:22
@barmac
Copy link
Member

barmac commented Oct 10, 2024

Congratz @philippfromme and @nikku for a successful migration

@philippfromme
Copy link
Contributor Author

Thanks for the help, guys! 💪🏻

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

Successfully merging this pull request may close these issues.

Support eslint@9
3 participants