feat(eslint): Upgrade eslint-config-yscope to v1.0.0 and fix lint issues.
#161
Conversation
WalkthroughThis pull request introduces a comprehensive set of changes primarily focused on code style and configuration updates across the project. The modifications include standardizing TypeScript interface property declarations by replacing commas with semicolons, updating ESLint configuration, and making minor formatting adjustments to type definitions and import statements. The changes do not significantly alter the functional aspects of the codebase but improve code consistency and readability. Changes
Suggested Reviewers
Possibly Related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
junhaoliao
force-pushed
the
new-eslint
branch
from
c51305b to
28f5d6a
Compare
# Conflicts: # package-lock.json # package.json # src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx
junhaoliao
changed the title
eslint-config-yscope to v1.0.0 and fix lint issues.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
eslint.config.mjs (1)
9-65: Consider adding section comments for better maintainability.While the configuration is well-structured, adding JSDoc comments before each major section would improve maintainability.
Example documentation:
+/** + * Base ESLint configuration array with various rule sets and overrides. + * @type {import('eslint').Linter.FlatConfig[]} + */ const EslintConfig = [ + // Ignore patterns for build and dependency directories { ignores: [
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (39)
eslint.config.mjs(1 hunks)jest.config.ts(1 hunks)package.json(2 hunks)src/components/CentralContainer/Sidebar/ResizeHandle.tsx(1 hunks)src/components/CentralContainer/Sidebar/SidebarTabs/CustomListItem.tsx(1 hunks)src/components/CentralContainer/Sidebar/SidebarTabs/CustomTabPanel.tsx(1 hunks)src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/Result.tsx(1 hunks)src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/ResultsGroup.tsx(1 hunks)src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx(1 hunks)src/components/CentralContainer/Sidebar/SidebarTabs/TabButton.tsx(1 hunks)src/components/CentralContainer/Sidebar/SidebarTabs/index.tsx(1 hunks)src/components/Editor/MonacoInstance/index.tsx(2 hunks)src/components/Editor/MonacoInstance/typings.ts(1 hunks)src/components/Editor/index.tsx(1 hunks)src/components/PopUps/PopUpMessageBox.tsx(2 hunks)src/components/StatusBar/LogLevelSelect/LogLevelChip.tsx(1 hunks)src/components/StatusBar/LogLevelSelect/index.tsx(2 hunks)src/components/modals/SettingsModal/index.tsx(1 hunks)src/contexts/NotificationContextProvider.tsx(2 hunks)src/contexts/StateContextProvider.tsx(4 hunks)src/contexts/UrlContextProvider.tsx(1 hunks)src/services/LogFileManager/index.ts(2 hunks)src/services/LogFileManager/utils.ts(1 hunks)src/services/MainWorker.ts(0 hunks)src/services/formatters/YscopeFormatter/utils.ts(1 hunks)src/typings/common.ts(1 hunks)src/typings/config.ts(1 hunks)src/typings/decoders.ts(3 hunks)src/typings/formatters.ts(3 hunks)src/typings/js.ts(1 hunks)src/typings/logs.ts(1 hunks)src/typings/notifications.ts(1 hunks)src/typings/url.ts(1 hunks)src/typings/worker.ts(3 hunks)src/utils/actions.ts(2 hunks)src/utils/data.ts(1 hunks)src/utils/http.ts(1 hunks)src/utils/units.ts(1 hunks)src/utils/url.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/services/MainWorker.ts
✅ Files skipped from review due to trivial changes (33)
- src/typings/common.ts
- src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/index.tsx
- jest.config.ts
- src/components/CentralContainer/Sidebar/SidebarTabs/CustomListItem.tsx
- src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/Result.tsx
- src/typings/notifications.ts
- src/components/CentralContainer/Sidebar/SidebarTabs/TabButton.tsx
- src/typings/decoders.ts
- src/components/Editor/index.tsx
- src/components/modals/SettingsModal/index.tsx
- src/services/formatters/YscopeFormatter/utils.ts
- src/components/CentralContainer/Sidebar/SidebarTabs/CustomTabPanel.tsx
- src/typings/logs.ts
- src/utils/http.ts
- src/typings/formatters.ts
- src/components/PopUps/PopUpMessageBox.tsx
- src/components/Editor/MonacoInstance/typings.ts
- src/components/CentralContainer/Sidebar/ResizeHandle.tsx
- src/typings/js.ts
- src/typings/url.ts
- src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/ResultsGroup.tsx
- src/components/StatusBar/LogLevelSelect/index.tsx
- src/utils/units.ts
- src/services/LogFileManager/index.ts
- src/components/CentralContainer/Sidebar/SidebarTabs/index.tsx
- src/utils/data.ts
- src/components/StatusBar/LogLevelSelect/LogLevelChip.tsx
- src/typings/worker.ts
- src/components/Editor/MonacoInstance/index.tsx
- src/services/LogFileManager/utils.ts
- src/contexts/UrlContextProvider.tsx
- src/contexts/StateContextProvider.tsx
- src/contexts/NotificationContextProvider.tsx
🧰 Additional context used
📓 Path-based instructions (3)
src/utils/url.ts (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
src/utils/actions.ts (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
src/typings/config.ts (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
🔇 Additional comments (10)
src/typings/config.ts (1)
29-34: LGTM! The interface declaration enhances extensibility.The conversion from type to interface is a good choice as it allows for declaration merging if needed in the future.
src/utils/actions.ts (4)
13-13: LGTM! Added trailing comma.The addition of a trailing comma is beneficial for cleaner diffs in version control.
16-19: LGTM! Consistent interface declaration.The conversion from type to interface aligns with the project-wide changes and enhances extensibility.
66-71: LGTM! Improved type formatting.The use of semicolons in type declarations improves consistency with TypeScript interface style.
76-79: LGTM! Enhanced readability.The improved indentation and formatting make the type declaration more readable.
eslint.config.mjs (3)
1-6: LGTM! Well-organized imports.The imports are properly structured and align with the package dependencies.
29-52: Well-structured import restrictions for MUI components.The import restrictions for MUI components are well-thought-out, promoting consistent usage patterns and preventing confusion.
68-68: LGTM! Proper export statement.The default export is correctly defined.
package.json (2)
53-53: LGTM! Required dependency for ESLint configuration.The addition of the
globalspackage is necessary for the Node.js globals configuration ineslint.config.mjs.
76-78: Verify typescript-eslint version compatibility.The override for typescript-eslint specifies version 8.19.0. Please ensure this version is compatible with the other TypeScript-related dependencies in the project.
Run this script to check version compatibility:
✅ Verification successful
The typescript-eslint override is correctly configured and compatible
The override is being used by eslint-config-yscope and is compatible with TypeScript 5.6.3.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check typescript-eslint version compatibility # Get TypeScript version from package.json echo "TypeScript version:" jq '.devDependencies.typescript' package.json # List all typescript-eslint related packages echo -e "\nTypeScript ESLint related packages:" jq '.dependencies + .devDependencies | with_entries(select(.key | contains("typescript-eslint")))' package.jsonLength of output: 307
Script:
#!/bin/bash # Check for typescript-eslint in the entire dependency tree echo "Checking typescript-eslint in dependency tree:" npm list typescript-eslint || true # Check if there are any ESLint configs that might be using typescript-eslint echo -e "\nChecking for ESLint configs:" fd -e js -e json -e yaml -e yml -e cjs --exec grep -l "typescript-eslint" {} || trueLength of output: 442
| } catch { | ||
| path = new URL(path, window.location.origin).href; | ||
| } |
There was a problem hiding this comment.
Improve error handling and align with documentation
The empty catch block silently handles errors, which:
- Contradicts the function's JSDoc that states it should throw an Error for invalid relative paths
- Makes debugging more difficult by discarding error information
- Deviates from the error handling pattern established in
getBasenameFromUrlOrDefault
Consider this implementation that aligns with the documentation and improves debugging:
try {
// eslint-disable-next-line no-new
new URL(path);
- } catch {
- path = new URL(path, window.location.origin).href;
+ } catch (e) {
+ try {
+ path = new URL(path, window.location.origin).href;
+ } catch (e) {
+ console.error(`Failed to parse URL: ${path}`, e);
+ throw new Error(`Invalid relative path: ${path}`);
+ }
}📝 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.
| } catch { | |
| path = new URL(path, window.location.origin).href; | |
| } | |
| try { | |
| // eslint-disable-next-line no-new | |
| new URL(path); | |
| } catch (e) { | |
| try { | |
| path = new URL(path, window.location.origin).href; | |
| } catch (e) { | |
| console.error(`Failed to parse URL: ${path}`, e); | |
| throw new Error(`Invalid relative path: ${path}`); | |
| } | |
| } |
package-lock.json
Outdated
There was a problem hiding this comment.
Do we really need to update all of these dependencies?
There was a problem hiding this comment.
Not necessarily but I think it's better to do a clean installation since we removed certain lint-related dependencies from package.json. i.e., instead of installing individual packages @typescript-eslint/eslint-plugin & @typescript-eslint/parser, now we rely on eslint-config-yscope to install its peer dependency typescript-eslint instead.
package.json
Outdated
| } | ||
| ] | ||
| } | ||
| "overrides": { |
There was a problem hiding this comment.
There's a bug in the latest version 8.19.1 (reported at typescript-eslint/typescript-eslint#10632) which causes lint failures. More details can be seen at #159
Since we do not directly specify the dependency typescript-eslint in our package.json (i.e., eslint-config-yscope list it as a peer dependency), to override the version to be installed, we can list the version here till they fix the bug. I expect them to fix that soon so this is a temporary measure.
Alternatively, we can also change eslint-config-yscope to have the typescript-eslint version locked (instead of using the ^ "compatible" directive in the version specification), so that we can avoid this version override in individual projects. What do you think?
There was a problem hiding this comment.
I think ur suggestion to change eslint-config-yscope it better. I will look at that PR.
eslint.config.mjs
Outdated
| ], | ||
| }, | ||
| CommonConfig, | ||
| ...TsConfigArray.map( |
There was a problem hiding this comment.
This map is a little strange looking, why can we not do the same thing we did with jest? Like files
files: ["test/**/*"], but for ts files
There was a problem hiding this comment.
It's tricky that we need to apply the files settings for individual config elements in the TsConfigArray array, while the JestConfig is an object.
That said, now I think about it again, I think it'd be better if we move this map into eslint-config-yscope since this should be universal to all individual projects. (i.e., it's unlikely we need to apply TS rules on any files that don't end with .ts or .tsx.
There was a problem hiding this comment.
I googled the documentation, and what you had originally is what they recommend doing. I think its okay as is.
|
For issues discussed above, here're the proposed changes in
then i'll make a release and update the code in this PR. |
|
Okay i dont think we need this y-scope/eslint-config-yscope#7 after googling more. So just merge y-scope/eslint-config-yscope#6, and y-scope/eslint-config-yscope#8. Then update this PR. Then I'll approve this for merging |
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
package.json (1)
TypeScript ESLint dependencies must be retained
The codebase actively uses TypeScript-specific ESLint rules (e.g.,
@typescript-eslint/prefer-literal-enum-member,@typescript-eslint/no-non-null-assertion). These dependencies are not included ineslint-config-yscopeand must be kept as direct dependencies.
- src/typings/config.ts: Uses
@typescript-eslint/prefer-literal-enum-member- src/index.tsx: Uses
@typescript-eslint/no-non-null-assertion🔗 Analysis chain
Line range hint
1-77: Verify the removal of typescript-eslint dependencies.The removal of
@typescript-eslint/eslint-pluginand@typescript-eslint/parserneeds verification as these might be required dependencies. Please confirm if these are now included ineslint-config-yscope.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if typescript-eslint dependencies are included in eslint-config-yscope echo "Checking eslint-config-yscope dependencies..." npm info eslint-config-yscope@latest dependencies --json | jq '."@typescript-eslint/eslint-plugin", ."@typescript-eslint/parser"' echo "Checking eslint-config-yscope peer dependencies..." npm info eslint-config-yscope@latest peerDependencies --json | jq '."@typescript-eslint/eslint-plugin", ."@typescript-eslint/parser"'Length of output: 489
Script:
#!/bin/bash # Find ESLint configuration files echo "ESLint configuration files:" fd -e js -e json -e yaml -e yml -g '.eslintrc*' # Check content of ESLint configuration files echo -e "\nESLint configuration contents:" for config in $(fd -e js -e json -e yaml -e yml -g '.eslintrc*'); do echo -e "\n=== $config ===" cat "$config" done # Check for any direct usage of typescript-eslint in the codebase echo -e "\nChecking for typescript-eslint usage in source files:" rg "@typescript-eslint" -lLength of output: 862
Script:
#!/bin/bash # Find ESLint configuration files (including hidden) echo "ESLint configuration files:" fd -H -e js -e json -e yaml -e yml -g '.eslintrc*' # Check content of ESLint configuration files echo -e "\nESLint configuration contents:" for config in $(fd -H -e js -e json -e yaml -e yml -g '.eslintrc*'); do echo -e "\n=== $config ===" cat "$config" done # Check the actual usage context echo -e "\nTypeScript file contents:" echo -e "\n=== src/typings/config.ts ===" cat src/typings/config.ts echo -e "\n=== src/index.tsx ===" cat src/index.tsxLength of output: 2092
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
package.json(1 hunks)
🔇 Additional comments (1)
package.json (1)
53-53: Verify the necessity of the globals package.The addition of the
globalspackage seems unnecessary as it should be included as a dependency ofeslint-config-yscope. Consider removing this direct dependency unless there's a specific need for it.✅ Verification successful
The globals package is required and correctly used
The package is necessary as it's actively used in
eslint.config.mjsto provide Node.js global variables for webpack configuration files. This is a legitimate use case and should be kept as a direct dependency.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if globals is already a dependency of eslint-config-yscope npm info eslint-config-yscope@latest dependencies --json | jq '."globals"'Length of output: 76
Script:
#!/bin/bash # Search for eslint-config-yscope package fd -t f "package.json" -x grep -l "eslint-config-yscope" # Search for direct usage of globals package rg -l "require\(['\"]globals['\"]\)" || rg -l "from ['\"](globals|globals/)['\"]" # Check if eslint-config-yscope is a local package fd -t f "eslint-config-yscope"Length of output: 209
Script:
#!/bin/bash # Check how globals is used in eslint.config.mjs cat eslint.config.mjsLength of output: 1888
| @@ -46,13 +46,11 @@ | |||
| "@types/node": "^22.9.0", | |||
| "@types/react": "^18.3.1", | |||
| "@types/react-dom": "^18.3.0", | |||
| "@typescript-eslint/eslint-plugin": "^7.13.0", | |||
| "@typescript-eslint/parser": "^7.13.0", | |||
| "babel-loader": "^9.1.3", | |||
| "copy-webpack-plugin": "^12.0.2", | |||
| "css-loader": "^7.1.2", | |||
| "eslint-config-yscope": "latest", | |||
There was a problem hiding this comment.
Avoid using the "latest" tag for eslint-config-yscope.
Using "latest" for eslint-config-yscope could lead to unexpected breaking changes. Consider pinning to a specific version for better stability and predictability.
Apply this diff to specify a fixed version:
- "eslint-config-yscope": "latest",
+ "eslint-config-yscope": "1.0.0",Committable suggestion skipped: line range outside the PR's diff.
…`; Simplified TypeScript config inclusion in `eslint.config.mjs` by directly spreading `TsConfigArray`.
Description
eslint-config-yscopeto v1.0.0.Validation performed
Summary by CodeRabbit
Based on the comprehensive pull request summary, here are the updated release notes:
Code Style Updates
Dependency Management
@typescript-eslint/eslint-pluginand@typescript-eslint/parser.globalsdependency.Minor Improvements
No significant functional changes were introduced in this release.