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

Update eslint #1664

Merged
merged 2 commits into from
Jan 22, 2025
Merged

Update eslint #1664

merged 2 commits into from
Jan 22, 2025

Conversation

kklamberty
Copy link
Member

@kklamberty kklamberty commented Jan 21, 2025

ESLint 8 has reached end of life, and it looks like it's important to update this to version 9 or more. Luckily, @NicMcPhee already did some work to modernize our eslint configuration, so I was able to basically just follow the migration guide here: https://eslint.org/docs/latest/use/migrate-to-9.0.0

However, I'm still not sure I have completed this task, nor do I know how to check. I think that the name of the file generated ending in .mjs might mean I need to do something more (at least rename that file to just .js at the end?) but I don't know. When I do rename the file to end in just .js rather than .mjs, I get some additional warning:

(node:34240) [MODULE_TYPELESS_PACKAGE_JSON] Warning: Module type of file:///Users/lamberty/Documents/3601/3601-iteration-template/client/eslint.config.js?mtime=1737432342100 is not specified and it doesn't parse as CommonJS.
Reparsing as ES module because module syntax was detected. This incurs a performance overhead.
To eliminate this warning, add "type": "module" to /Users/lamberty/Documents/3601/3601-iteration-template/client/package.json.
(Use node --trace-warnings ... to show where the warning was created)

I don't know if that warning tells me more (like, hey, you're actually linting now), or if leaving it as mjs is preferred. Also, the file contains more ... than I would have expected. I don't know what those mean. I see references about it being the spread operator and about it being used to indicate things that are overwritten. The overwriting idea makes some sense in this context... like you will be given some default setting, and instead you will use this. But, I'd still like to have input from others on this PR in case I've done something wildly incorrect or I've somehow turned off linting. Any thoughts?

I installed a node deprecation checker (npm install npm-deprecated-check) and used ndc current to check the installation, and that made it seem like I have maybe at least succeeded in removing the deprecated eslint.

@kklamberty kklamberty added help wanted Extra attention is needed question Further information is requested dependencies Pull requests that update a dependency file labels Jan 21, 2025
@kklamberty kklamberty marked this pull request as ready for review January 21, 2025 04:14
@kklamberty kklamberty requested a review from NicMcPhee January 21, 2025 04:14
kklamberty added a commit that referenced this pull request Jan 21, 2025
https://typescript-eslint.io/rules/no-require-imports/
There was a complaint from ESLint about the use of require in the `cypress.config.ts` file.

There is a Cypress plugin for ESLint https://github.com/cypress-io/eslint-plugin-cypress and this approach to adding it to the `.eslintrc.json` is suggested in the history of the project. The project claims you need to be using ESLint 9 or greater, which we probably should look into, and it might mean that this plugin would need to be updated manually in that migration process. I have a PR for that migration as well:
#1664
Copy link
Member

@NicMcPhee NicMcPhee left a comment

Choose a reason for hiding this comment

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

I think the .mjs is just a a JS module file, taking advantage of some newer JS features. It basically just moved the rules in .eslintrc.json into that JS module file.

The ... is similar to the spread operator in Python if you've used that. I don't know if I've ever used it in JS/TS, but we could potentially file it useful in places?

It would probably make sense to sit down and go over (and document) the contents of this file to make it easier to understand and maintain downstream.

@kklamberty kklamberty added this pull request to the merge queue Jan 22, 2025
Merged via the queue into main with commit 512b7fe Jan 22, 2025
17 checks passed
@kklamberty kklamberty deleted the update-eslint branch February 6, 2025 05:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file help wanted Extra attention is needed question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants