-
Notifications
You must be signed in to change notification settings - Fork 77
eslint 9 update #1908
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
base: main
Are you sure you want to change the base?
eslint 9 update #1908
Conversation
82619df
to
d882a27
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 43 out of 55 changed files in this pull request and generated no comments.
Files not reviewed (12)
- .editorconfig: Language not supported
- .eslintignore: Language not supported
- .eslintrc.json: Language not supported
- .nvmrc: Language not supported
- .vscode/settings.json: Language not supported
- examples/arithmetics/package.json: Language not supported
- examples/domainmodel/package.json: Language not supported
- examples/requirements/package.json: Language not supported
- examples/statemachine/package.json: Language not supported
- package.json: Language not supported
- packages/generator-langium/package.json: Language not supported
- packages/generator-langium/templates/cli/.package.json: Language not supported
Comments suppressed due to low confidence (2)
packages/generator-langium/src/index.ts:184
- The variable name 'path' shadows the imported Node 'path' module. Consider renaming it (e.g., 'targetPath') to avoid potential conflicts or confusion.
for (const path of ['.', '.vscode', 'eslint.config.mjs']) {
examples/statemachine/esbuild.mjs:1
- Removing the //@ts-check comment disables inline type-checking in this file; ensure that your build process or IDE still enforces the desired type safety.
-//@ts-check
- Leave yeoman generator and generator-test on previous version - TypeScript compiler target is ES2020. The library code is aligned
d882a27
to
e6ab4bb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 36 out of 50 changed files in this pull request and generated no comments.
Files not reviewed (14)
- .editorconfig: Language not supported
- .eslintignore: Language not supported
- .eslintrc.json: Language not supported
- .nvmrc: Language not supported
- .vscode/settings.json: Language not supported
- examples/arithmetics/package.json: Language not supported
- examples/domainmodel/package.json: Language not supported
- examples/requirements/package.json: Language not supported
- examples/statemachine/package.json: Language not supported
- package.json: Language not supported
- packages/generator-langium/package.json: Language not supported
- packages/generator-langium/templates/cli/.package.json: Language not supported
- packages/generator-langium/templates/core/.eslintrc.json: Language not supported
- packages/generator-langium/templates/core/.package.json: Language not supported
Comments suppressed due to low confidence (3)
packages/generator-langium/templates/web/src/setupClassic.ts:13
- Replacing a template literal with a single-quoted string may remove future interpolation support. Verify that this change is intentional and that no dynamic content is expected.
code: '// <%= RawLanguageName %> is running in the web!',
packages/generator-langium/src/index.ts:184
- Ensure that the update from '.eslintrc.json' to 'eslint.config.mjs' correctly reflects your project's ESLint configuration file location and that all references to the old config are removed.
for (const path of ['.', '.vscode', 'eslint.config.mjs']) {
examples/requirements/src/cli/generator.ts:19
- Removing the eslint-disable directive for indent may reintroduce formatting issues if the ESLint indent rule is enforced; please confirm that the generated HTML content remains properly formatted.
/* eslint-disable @typescript-eslint/indent */
Next time I will separate compiler update, eslint update and required dependencies into separate PRs. From the recent upgrade experience in OCT I thought this is an straightforward task, but larger code bases bring more complexity. I know that and should not fall into the trap. 🙂 Implementation becomes harder and the review as well. My apologies for making this review more complex than needed. |
The eslint configuration of open-collaboration-tools has been the baseline for this implementation. Specific adjustment from the old configuration have been added.
The TypeScript configuration has been clean and aligned with OCT as well.
I needed to lower the compiler target to
ES2020
because otherwise almost half of the unit test using the chevrotain parser failed. The lib reference was lowered fromESNEXT
toES2020
as well to ensure lib and target are in-line. This required some code changes, becausestring.replaceAll
is only available afterES2021
.This PR also bumps the minimum engines up to node 20.
ES2022
/ES2023
should be valid TS/node20 target according to this list from TypeScript, but it doesn't help if our dependency stacks doesn't work with (potential chevrotain issue/investigation required?).All dependencies but yeoman-generator were updated to current versions, because of incompatible API changes. This should be tracked in a new issue.
One hack remains: I can't set
'no-useless-escape': 'off'
inherited rule fromeslint:recommended
, although it should it work. 🤷♂️ It is taken into account, because once I configure an illegal string eslint does not start. I excluded the generated syntaxes/*.monarch.ts files from processing. The real cause/[/\*]/
(backslah not required) in the monarch generator should be fixed at one point, but still the config override must work. Update: I just saw those files were contained in the deleted eslintignore as well, so the problem is not new.Fixes #1904
Fixes #1905