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.

Sign up for GitHub

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

Jump to bottom

Migrate to ESLint 9 and flat config #936

Merged
merged 7 commits into from
Oct 14, 2024
Merged

Migrate to ESLint 9 and flat config #936

merged 7 commits into from
Oct 14, 2024

Conversation

philippfromme
Copy link
Contributor

@philippfromme philippfromme commented Oct 7, 2024 •
edited
Loading

Related to https://github.com/bpmn-io/internal-docs/issues/1042

Proposed Changes

  • updates ESLint to v9
  • updates eslint-plugin-bpmn-io to v2
  • removes eslint-plugin-import
  • fixes newly reported lint errors

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}

@bpmn-io-tasks bpmn-io-tasks bot added the in progress Currently worked on label Oct 7, 2024
@nikku
Copy link
Member

nikku commented Oct 10, 2024 •
edited
Loading

I've update this PR to use eslint-plugin-bpmn-io@2, and it found additional violations that are reasonable:

image

Some minor adjustments for the mocha validations were needed, I propose to incorporate them in the plug-in in a patch version.

My investigation is completed for the moment, @philippfromme consider when and how to take this back over.

@barmac
Copy link
Member

barmac commented Oct 10, 2024

The nested test looks like a false positive. We should change the function name only.

@nikku nikku mentioned this pull request Oct 10, 2024
Merged
4 tasks
@nikku
Copy link
Member

nikku commented Oct 11, 2024

Updated to eslint-plugin-bpmn-io@2.0.1.

@nikku nikku mentioned this pull request Oct 11, 2024
Closed
1 task
@nikku nikku added the dependencies Updates a dependency label Oct 11, 2024
@philippfromme
Copy link
Contributor Author

So I guess we want to fix things like no-identical-title and not disable that rule? We have the same issue in the Camunda Modeler where we would have to fix a couple of things.

@nikku
Copy link
Member

nikku commented Oct 11, 2024

@philippfromme Yes. As mentioned in #936 (comment) I think the left over errors are quality of life.

@philippfromme philippfromme marked this pull request as ready for review October 11, 2024 12:00
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels Oct 11, 2024
@philippfromme philippfromme requested review from nikku, barmac and jarekdanielak and removed request for nikku October 11, 2024 12:00
barmac
barmac reviewed Oct 11, 2024
View reviewed changes
eslint.config.mjs Outdated Show resolved Hide resolved
nikku
nikku reviewed Oct 11, 2024
View reviewed changes
test/spec/features/overlays/OverlaysSpec.js
@@ -490,7 +490,7 @@ describe('features/overlays', function() {
}


it('should position top left of shape', inject(function(overlays) {
it(' (connection)', inject(function(overlays) {
Copy link
Member

Choose a reason for hiding this comment

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

Is this an accident?

nikku
nikku reviewed Oct 11, 2024
View reviewed changes
test/spec/features/resize/ResizeSpec.js
@@ -313,29 +313,6 @@ describe('features/resize - Resize', function() {
}));


it('should resize to minimum bounds', inject(function(canvas, resize, dragging, elementFactory) {
Copy link
Member

Choose a reason for hiding this comment

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

Removed by accident, or duplicate?

nikku
nikku reviewed Oct 11, 2024
View reviewed changes
test/spec/features/resize/ResizeUtilSpec.js
@@ -329,61 +329,6 @@ describe('resize/ResizeUtil', function() {
});


describe('getMinResizeBounds', function() {
Copy link
Member

Choose a reason for hiding this comment

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

Removed by accident or duplicate?

nikku
nikku reviewed Oct 11, 2024
View reviewed changes
test/spec/i18n/translate/translateSpec.js
@@ -38,11 +38,6 @@ describe('i18n - translate', function() {
expect(translate('<div />')).to.eql('<div />');
}));


it('should handle missing replacement', inject(function(translate) {
Copy link
Member

Choose a reason for hiding this comment

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

Duplicate ✔️

@nikku
Copy link
Member

nikku commented Oct 11, 2024

@philippfromme Updated this PR as discussed with proper configuration of browser vs. node environments (ce60276). After you do it, then you see that 85027fe is indeed necessary. No-one but us defines that global.

@nikku
Copy link
Member

nikku commented Oct 11, 2024

Updated to designated eslint format: 4b17955.

@nikku nikku mentioned this pull request Oct 14, 2024
Closed
1 task
@nikku
Copy link
Member

nikku commented Oct 14, 2024

@philippfromme Some open questions on this PR.

@philippfromme
Copy link
Contributor Author

@nikku The deleted tests were duplicates.

@nikku nikku merged commit 5144004 into develop Oct 14, 2024
11 of 12 checks passed
@nikku nikku deleted the eslint-9 branch October 14, 2024 12:49
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikku nikku nikku left review comments

@barmac barmac barmac left review comments

@jarekdanielak jarekdanielak Awaiting requested review from jarekdanielak

Assignees

@philippfromme philippfromme

Labels
dependencies Updates a dependency
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@philippfromme @nikku @barmac