-
Notifications
You must be signed in to change notification settings - Fork 74
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: replace babel-eslint with @babel/eslint-parser #812
Conversation
WalkthroughThe changes involve significant updates to the ESLint configuration and related dependencies within the project. The Changes
Poem
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 (
|
Signed-off-by: Yi-Ya Chen <[email protected]>
b9a6c86
to
5c80080
Compare
"ecmaFeatures": { | ||
"jsx": true, | ||
"experimentalObjectRestSpread": true |
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.
@@ -78,12 +78,11 @@ | |||
"css-loader": "^2.1.1", | |||
"cssnano": "^4.1.10", | |||
"es5-imcompatible-versions": "^0.1.34", | |||
"eslint": "^5.16.0", | |||
"eslint-config-airbnb": "^17.1.0", | |||
"eslint-plugin-compat": "^2.2.0", |
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.
I don't think this is used in the code.
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.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
.eslintrc (1)
Line range hint
12-87
: Document the temporary nature of disabled rulesWhile disabling these rules is intentional as per PR objectives (pending Prettier integration), it would be helpful to document this in the configuration file itself to prevent these temporary changes from becoming permanent.
Add a comment at the top of the rules section:
"rules": { + // Note: Many rules are temporarily disabled pending Prettier integration "semi": [ 2, "never" ],
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.json
is excluded by!**/package-lock.json
📒 Files selected for processing (2)
.eslintrc
(2 hunks)package.json
(2 hunks)
🔇 Additional comments (4)
.eslintrc (2)
89-95
: LGTM: Parser and ECMAScript version updates
The changes align well with the PR objectives:
- Upgrade to
@babel/eslint-parser
is the main goal - ECMAScript 2020 support enables modern JavaScript features
96-96
: Verify impact of disabling Babel config requirement
Setting requireConfigFile: false
bypasses the need for a Babel configuration file. While this simplifies setup, it could lead to unexpected parsing behavior if Babel-specific features are used.
Let's check if there are any Babel-specific features in use:
package.json (2)
81-85
: Consider upgrading to ESLint 8.x and verify breaking changes
The updates look compatible, but there are a few considerations:
- ESLint 7.x is approaching end-of-life. Consider upgrading to ESLint 8.x in a future PR.
- The upgrade from eslint-config-airbnb 17.x to 18.x might introduce breaking changes.
#!/bin/bash
# Check for breaking changes in eslint-config-airbnb
echo "Checking eslint-config-airbnb changelog for breaking changes between v17 and v18:"
gh api repos/airbnb/javascript/releases | jq -r '.[] | select(.tag_name | startswith("eslint-config-airbnb-v18")) | .body'
# Verify peer dependency requirements
echo "\nVerifying peer dependencies:"
npm info [email protected] peerDependencies
Consider creating a separate PR for upgrading to ESLint 8.x after this change is stable. This would help isolate any potential issues and make the changes more manageable.
43-43
: Verify @babel/eslint-parser version and consider upgrading
While the migration from babel-eslint
to @babel/eslint-parser
is correct, version 7.25.9
is relatively old.
✅ Verification successful
@babel/eslint-parser version is up-to-date and secure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for latest version and any security advisories
echo "Latest version from npm:"
npm view @babel/eslint-parser version
echo "\nChecking for security advisories:"
npm audit --json @babel/[email protected] | jq '.advisories'
Length of output: 350
Script:
#!/bin/bash
# Check for latest version and any security advisories
echo "Latest version from npm:"
npm view @babel/eslint-parser version
echo -e "\nChecking for security advisories:"
npm audit --json @babel/[email protected] | jq '.advisories'
Length of output: 257
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.
What this PR does / why we need it
Replaces
babel-eslint
with@babel/eslint-parser
, which requiresESLint 7.5.0
or higher.The bot PR failed, so we’re upgrading ESLint manually in this PR.
Issue
#776
Test Result
0 error from
npm run lint
Additional documentation or context
Some new rules are disabled to avoid many file changes in this PR.
They can be reviewed again when we introduce Prettier in the future.
Summary by CodeRabbit