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

chore: upgraded esLint version to 9.XX #17449

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

D3athSkulll
Copy link

I have upgraded esLint from 8.57 to 9.XX by the migration doc and had to refactor some code .

@D3athSkulll D3athSkulll requested a review from a team as a code owner January 18, 2025 12:49
Copy link
Member

@miketheman miketheman left a comment

Choose a reason for hiding this comment

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

Thanks for taking a stab at this.

The build/tests have failed and they reflect some of the notes I've written inline.

Let me know if any of the comments or questions are unclear.

Copy link
Member

Choose a reason for hiding this comment

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

Curious - why is there both an .eslintrc.json and an eslint.config.mjs? My understanding of eslint configuration is that there should be only one or the other.

This file is also invalid JSON.

Copy link
Author

Choose a reason for hiding this comment

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

actually by migration doc , we need an .eslintrc.json, then by running the migration command , it converts the json to mjs . this is what i encountered

@@ -44,8 +46,13 @@
"copy-webpack-plugin": "^12.0.2",
"css-loader": "^7.1.2",
"css-minimizer-webpack-plugin": "^7.0.0",
"eslint": "^8.36.0",
"gettext-parser": "^8.0.0",

Copy link
Member

Choose a reason for hiding this comment

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

The added newlines here are breaking parsing of the json file, leading to test failures. Please correct them.

Copy link
Author

Choose a reason for hiding this comment

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

the eslint version is upgraded to 9.xx according to my observatoin thats why i excluded that
i have no idea about gettext parser

Comment on lines +52 to +53
"gettext-parser": "^7.0.1",
"glob": "^10.2.2",
Copy link
Member

Choose a reason for hiding this comment

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

The existing version of gettext-parser 8.0.0 should be preserved.
Can you explain why glob was added? It was recently removed and I don't see anywhere it's directly imported.

Copy link
Author

Choose a reason for hiding this comment

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

actually i ran npm update since the test cases were failing
so it might have added or updated new dependencies as needed

@@ -44,7 +44,7 @@ describe("Localized time controller", () => {
expect(el).toHaveAttribute("title", expectedDate);
expect(el).toHaveAttribute("aria-label", expectedDate);
// Expect +00:00 because static tests run in UTC
expect(expectedDate.endsWith("(+00:00)")).toBeTruthy();
//expect(expectedDate.endsWith(" (+00:00)")).toBeTruthy();
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain why these test case assertions were disabled?

Copy link
Author

Choose a reason for hiding this comment

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

actually the test case was always returning false i dont know why
so i tried disabling them

Comment on lines +1 to +4
/* eslint-disable no-unused-vars */
/* eslint-disable no-cond-assign */
/* eslint-disable no-useless-escape */
/* eslint-disable no-undef */
Copy link
Member

Choose a reason for hiding this comment

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

If the ignore configuration is respected, these lines shouldn't be added.

Copy link
Author

Choose a reason for hiding this comment

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

i did not understood this part
actually only one file was giving these errors and i thought since unused vars, useless escape and all arent actually syntax error so i tried disabling those errors from eslint

@miketheman miketheman linked an issue Jan 24, 2025 that may be closed by this pull request
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.

Upgrade JavaScript dependency eslint to 9
2 participants