chore(dependencies): update eslint 8->9 #296
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #296 +/- ##
========================================
Coverage 72.45% 72.45%
========================================
Files 40 40
Lines 10153 10155 +2
Branches 586 586
========================================
+ Hits 7356 7358 +2
Misses 2793 2793
Partials 4 4 ☔ View full report in Codecov by Sentry. |
djwhitt
force-pushed
the
feature/PE-7535_major_1
branch
from
3415a8b to
1853eb1
Compare
djwhitt
force-pushed
the
feature/PE-7535_major_2
branch
from
fb1dcb8 to
06b382c
Compare
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
error @permaweb/[email protected]: The engine "yarn" is incompatible with this module. Expected version "please-use-npm". Got "1.22.22" 📝 WalkthroughWalkthroughThis pull request introduces a comprehensive update to the ESLint configuration for the project. The primary changes include replacing the legacy Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
eslint.config.mjs (1)
49-73: Document rationale for disabled rules.Several TypeScript and ESLint rules are disabled without explanation:
@typescript-eslint/no-explicit-any@typescript-eslint/no-unused-varsno-unused-varsno-consoleConsider adding comments explaining why these rules are disabled or enable them if possible.
src/store/lmdb-kv-store.test.ts (1)
44-44: Consider using type guards instead of non-null assertions.While the non-null assertions are safe here due to the preceding checks, consider using type guards for better type safety:
- assert.equal(toB64Url(result!), 'test'); + if (result) { + assert.equal(toB64Url(result), 'test'); + }This pattern would be more explicit and type-safe, eliminating the need for non-null assertions.
Also applies to: 64-64, 77-77
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (13)
.eslintrc(0 hunks)eslint.config.mjs(1 hunks)package.json(2 hunks)src/data/ar-io-data-source.ts(1 hunks)src/data/gateways-data-source.test.ts(1 hunks)src/data/read-through-data-cache.test.ts(4 hunks)src/lib/validation.ts(1 hunks)src/store/kv-debounce-store.test.ts(6 hunks)src/store/lmdb-kv-store.test.ts(3 hunks)src/workers/sqlite-wal-cleanup-worker.ts(1 hunks)src/workers/transaction-offset-repair-worker.ts(1 hunks)src/workers/transaction-repair-worker.ts(1 hunks)test/sqlite-helpers.ts(0 hunks)
💤 Files with no reviewable changes (2)
- test/sqlite-helpers.ts
- .eslintrc
✅ Files skipped from review due to trivial changes (4)
- src/data/ar-io-data-source.ts
- src/store/kv-debounce-store.test.ts
- src/data/read-through-data-cache.test.ts
- src/data/gateways-data-source.test.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test (macos-latest)
- GitHub Check: test (ubuntu-latest)
🔇 Additional comments (5)
eslint.config.mjs (1)
15-16: Consider tracking the header plugin workaround.The workaround for the header plugin's schema validation might need to be removed once issue #57 is resolved.
✅ Verification successful
Workaround is the current recommended solution
The workaround for the header plugin's schema validation is the officially recommended solution, as confirmed in the issue discussion. While there are ongoing efforts to implement a proper fix, this approach remains the standard solution for ESLint v9 compatibility.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the issue is still open gh issue view 57 --repo Stuk/eslint-plugin-headerLength of output: 1407
Script:
#!/bin/bash # Check the latest comments on the issue gh issue view 57 --repo Stuk/eslint-plugin-header --commentsLength of output: 7596
src/workers/sqlite-wal-cleanup-worker.ts (1)
52-54: LGTM! Improved readability of interval clearing.The explicit if statement is clearer than the previous ternary operator.
src/workers/transaction-repair-worker.ts (1)
57-59: LGTM! Consistent with other interval clearing changes.The explicit if statement matches the pattern used in other workers.
src/workers/transaction-offset-repair-worker.ts (1)
54-56: LGTM! Maintains consistency across workers.The explicit if statement follows the same pattern used in other worker classes.
package.json (1)
58-59: Verify compatibility between updated dependencies.The ESLint upgrade includes several major version jumps:
- ESLint: v8 → v9
- @typescript-eslint: v5 → v8
- eslint-plugin-unicorn: v49 → v56
Let's verify the compatibility of these versions:
Additionally, ensure that:
- All ESLint rules still work as expected after the upgrade
- The new ESLint configuration is tested across different file types
- CI pipeline includes linting checks
Also applies to: 77-78, 81-82, 86-86
✅ Verification successful
Dependencies compatibility verified ✓
All version requirements are met:
- ESLint v9 is supported by all plugins
- TypeScript v5.7.3 is compatible with @typescript-eslint v8
- No deprecation notices found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any peer dependency warnings or compatibility issues npm info [email protected] peerDependencies npm info @typescript-eslint/[email protected] peerDependencies npm info [email protected] peerDependencies # Check if there are any deprecation notices npm view [email protected] deprecated npm view @typescript-eslint/[email protected] deprecated npm view [email protected] deprecatedLength of output: 477
Script:
#!/bin/bash # Extract typescript version from package.json jq -r '.dependencies.typescript // .devDependencies.typescript' package.jsonLength of output: 85
| languageOptions: { | ||
| parser: tsParser, | ||
| ecmaVersion: 5, | ||
| sourceType: 'script', | ||
|
|
||
| parserOptions: { | ||
| project: 'tsconfig.json', | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Update language options for modern TypeScript.
The current configuration might be too restrictive:
ecmaVersion: 5is outdated for a TypeScript projectsourceType: 'script'could cause issues with ES modules
Apply this diff to align with modern TypeScript practices:
languageOptions: {
parser: tsParser,
- ecmaVersion: 5,
- sourceType: 'script',
+ ecmaVersion: 2022,
+ sourceType: 'module',
parserOptions: {
project: 'tsconfig.json',
},
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| languageOptions: { | |
| parser: tsParser, | |
| ecmaVersion: 5, | |
| sourceType: 'script', | |
| parserOptions: { | |
| project: 'tsconfig.json', | |
| }, | |
| }, | |
| languageOptions: { | |
| parser: tsParser, | |
| ecmaVersion: 2022, | |
| sourceType: 'module', | |
| parserOptions: { | |
| project: 'tsconfig.json', | |
| }, | |
| }, |
| @@ -44,7 +44,7 @@ export function sanityCheckBlock(block: PartialJsonBlock) { | |||
| throw new Error("Invalid block: invalid 'indep_hash' format"); | |||
| } | |||
|
|
|||
| if (!block.height === undefined) { | |||
| if (!block.height === false) { | |||
There was a problem hiding this comment.
Fix the block height validation logic.
The current condition if (!block.height === false) is confusing and potentially error-prone. It's equivalent to checking if block.height is truthy, which isn't the intended validation.
Consider this clearer alternative:
- if (!block.height === false) {
+ if (block.height === undefined) {
throw new Error("Invalid block: missing 'height'");
}This explicitly checks for undefined, making the validation more straightforward and maintainable.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!block.height === false) { | |
| if (block.height === undefined) { |
|
@hlolli I'm in favor of upgrading, but I get the following when I run |
djwhitt
force-pushed
the
feature/PE-7535_major_2
branch
from
06b382c to
392cb79
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (13)
.eslintrc(0 hunks)eslint.config.mjs(1 hunks)package.json(2 hunks)src/data/ar-io-data-source.ts(1 hunks)src/data/gateways-data-source.test.ts(1 hunks)src/data/read-through-data-cache.test.ts(4 hunks)src/lib/validation.ts(1 hunks)src/store/kv-debounce-store.test.ts(6 hunks)src/store/lmdb-kv-store.test.ts(3 hunks)src/workers/sqlite-wal-cleanup-worker.ts(1 hunks)src/workers/transaction-offset-repair-worker.ts(1 hunks)src/workers/transaction-repair-worker.ts(1 hunks)test/sqlite-helpers.ts(0 hunks)
💤 Files with no reviewable changes (2)
- test/sqlite-helpers.ts
- .eslintrc
🚧 Files skipped from review as they are similar to previous changes (10)
- src/workers/transaction-repair-worker.ts
- src/workers/sqlite-wal-cleanup-worker.ts
- src/data/ar-io-data-source.ts
- src/store/kv-debounce-store.test.ts
- src/lib/validation.ts
- src/data/gateways-data-source.test.ts
- src/workers/transaction-offset-repair-worker.ts
- src/store/lmdb-kv-store.test.ts
- src/data/read-through-data-cache.test.ts
- package.json
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test (macos-latest)
- GitHub Check: test (ubuntu-latest)
🔇 Additional comments (4)
eslint.config.mjs (4)
1-21: LGTM! Note about the header plugin workaround.The imports and setup are well-organized. The workaround for the header plugin schema issue is properly documented with a reference to the GitHub issue.
24-29: LGTM! Configuration extends are well-structured.The configuration properly extends the recommended configs in the correct order: ESLint base, TypeScript ESLint, and Prettier.
39-47: Update language options for modern TypeScript.The current configuration is too restrictive for a modern TypeScript project.
Apply this diff to align with modern TypeScript practices:
languageOptions: { parser: tsParser, - ecmaVersion: 5, - sourceType: 'script', + ecmaVersion: 2022, + sourceType: 'module', parserOptions: { project: 'tsconfig.json', }, },
1-6: Verify TypeScript version compatibility.The PR comments indicate a version mismatch between TypeScript (5.7.3) and @typescript-eslint's supported versions (3.3.1 to 5.2.0).
Run this script to check compatibility:
✅ Verification successful
TypeScript version 5.7.3 is compatible with @typescript-eslint
The @typescript-eslint packages support TypeScript versions from 4.8.4 up to (but not including) 5.8.0, which means TypeScript 5.7.3 is fully supported.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check TypeScript and @typescript-eslint versions # Get installed TypeScript version echo "Installed TypeScript version:" npm list typescript # Get @typescript-eslint peer dependencies echo -e "\n@typescript-eslint peer dependencies:" npm info "@typescript-eslint/parser" peerDependencies npm info "@typescript-eslint/eslint-plugin" peerDependencies # Get ESLint version echo -e "\nInstalled ESLint version:" npm list eslintLength of output: 656
| rules: { | ||
| '@typescript-eslint/no-explicit-any': ['off'], | ||
| '@typescript-eslint/no-unused-vars': ['off'], | ||
|
|
||
| '@typescript-eslint/strict-boolean-expressions': [ | ||
| 2, | ||
| { | ||
| allowNullableObject: true, | ||
| allowNullableBoolean: true, | ||
| allowAny: true, | ||
| }, | ||
| ], | ||
|
|
||
| eqeqeq: ['error', 'smart'], | ||
| 'jest-formatting/padding-around-describe-blocks': 2, | ||
| 'jest-formatting/padding-around-test-blocks': 2, | ||
| 'header/header': [2, './resources/license.header.js'], | ||
| 'mocha/max-top-level-suites': 'off', | ||
| 'mocha/no-exports': 'off', | ||
| 'mocha/no-mocha-arrows': 'off', | ||
| 'no-console': 0, | ||
| 'no-return-await': 2, | ||
| 'no-unneeded-ternary': 2, | ||
| 'no-unused-vars': 'off', | ||
|
|
||
| 'prettier/prettier': [ | ||
| 'error', | ||
| { | ||
| endOfLine: 'auto', | ||
| }, | ||
| ], | ||
|
|
||
| 'unicorn/prefer-node-protocol': 2, | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add rules to address reported linting issues.
Based on the PR comments, there are several linting issues that should be addressed:
- prefer-const violations
- non-null assertion warnings
Add these rules to enforce better code quality:
rules: {
'@typescript-eslint/no-explicit-any': ['off'],
'@typescript-eslint/no-unused-vars': ['off'],
+ '@typescript-eslint/no-non-null-assertion': ['warn'],
+ 'prefer-const': ['error'],
'@typescript-eslint/strict-boolean-expressions': [📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| rules: { | |
| '@typescript-eslint/no-explicit-any': ['off'], | |
| '@typescript-eslint/no-unused-vars': ['off'], | |
| '@typescript-eslint/strict-boolean-expressions': [ | |
| 2, | |
| { | |
| allowNullableObject: true, | |
| allowNullableBoolean: true, | |
| allowAny: true, | |
| }, | |
| ], | |
| eqeqeq: ['error', 'smart'], | |
| 'jest-formatting/padding-around-describe-blocks': 2, | |
| 'jest-formatting/padding-around-test-blocks': 2, | |
| 'header/header': [2, './resources/license.header.js'], | |
| 'mocha/max-top-level-suites': 'off', | |
| 'mocha/no-exports': 'off', | |
| 'mocha/no-mocha-arrows': 'off', | |
| 'no-console': 0, | |
| 'no-return-await': 2, | |
| 'no-unneeded-ternary': 2, | |
| 'no-unused-vars': 'off', | |
| 'prettier/prettier': [ | |
| 'error', | |
| { | |
| endOfLine: 'auto', | |
| }, | |
| ], | |
| 'unicorn/prefer-node-protocol': 2, | |
| }, | |
| rules: { | |
| '@typescript-eslint/no-explicit-any': ['off'], | |
| '@typescript-eslint/no-unused-vars': ['off'], | |
| '@typescript-eslint/no-non-null-assertion': ['warn'], | |
| 'prefer-const': ['error'], | |
| '@typescript-eslint/strict-boolean-expressions': [ | |
| 2, | |
| { | |
| allowNullableObject: true, | |
| allowNullableBoolean: true, | |
| allowAny: true, | |
| }, | |
| ], | |
| eqeqeq: ['error', 'smart'], | |
| 'jest-formatting/padding-around-describe-blocks': 2, | |
| 'jest-formatting/padding-around-test-blocks': 2, | |
| 'header/header': [2, './resources/license.header.js'], | |
| 'mocha/max-top-level-suites': 'off', | |
| 'mocha/no-exports': 'off', | |
| 'mocha/no-mocha-arrows': 'off', | |
| 'no-console': 0, | |
| 'no-return-await': 2, | |
| 'no-unneeded-ternary': 2, | |
| 'no-unused-vars': 'off', | |
| 'prettier/prettier': [ | |
| 'error', | |
| { | |
| endOfLine: 'auto', | |
| }, | |
| ], | |
| 'unicorn/prefer-node-protocol': 2, | |
| }, |
|
@djwhitt interesting, I missed a bunch, I must have been testing this locally weirdly because I had lint errors with yarn lint:check but I fixed them all (or so I believed)
if we don't have bug it's probably ok, I notice this often |
Maybe we just bump the version on that as part of this. Perhaps it's included in the other dependency update PR as well. I didn't check yet. |
|
Also, probably worth mentioning I really just want to switch to Biome instead... 😅 |
following the migration guide https://eslint.org/docs/latest/use/configure/migration-guide
added two packages that the migration required, ran a migration script that moved .eslintrc to a new eslint mjs file. Some automated (and few manual) linter fixes.