-
Notifications
You must be signed in to change notification settings - Fork 13k
chore(core-services): Review build configuration #37925
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
Conversation
|
WalkthroughRemoved Babel configuration and related tooling from the core-services package. Updated ESLint configuration to remove Jest-specific environment settings for spec files, simplified lint commands, added a Jest test script, and removed Babel-related devDependencies. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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: 1
🧹 Nitpick comments (1)
packages/core-services/package.json (1)
15-16: Optional: Consider consolidating duplicate test scripts.Both
testandtestunitscripts run the same command (jest). While harmless, you might consider removingtestunitif it's no longer needed, or document why both exist if they serve different purposes in your workflow.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
packages/core-services/.eslintrc.jsonpackages/core-services/babel.config.jspackages/core-services/package.json
💤 Files with no reviewable changes (2)
- packages/core-services/.eslintrc.json
- packages/core-services/babel.config.js
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37547
File: packages/i18n/src/locales/en.i18n.json:634-634
Timestamp: 2025-11-19T12:32:29.696Z
Learning: Repo: RocketChat/Rocket.Chat
Context: i18n workflow
Learning: In this repository, new translation keys should be added to packages/i18n/src/locales/en.i18n.json only; other locale files are populated via the external translation pipeline and/or fall back to English. Do not request adding the same key to all locale files in future reviews.
📚 Learning: 2025-12-10T21:00:54.909Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
Applied to files:
packages/core-services/package.json
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to **/*.{ts,tsx,js} : Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Applied to files:
packages/core-services/package.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (1)
packages/core-services/package.json (1)
15-16: The test scripts will run successfully with the current configuration. TypeScript transformation is handled by @swc/jest, which is already included as a dependency in @rocket.chat/jest-presets and properly inherited by the core-services jest configuration.Likely an incorrect or invalid review comment.
| "lint": "eslint .", | ||
| "lint:fix": "eslint --fix .", |
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.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify ESLint configuration for Jest globals
# Check the base ESLint config for Jest support
echo "=== Checking @rocket.chat/eslint-config for Jest support ==="
find . -path "*/eslint-config*" -name "*.js" -o -path "*/eslint-config*" -name ".eslintrc*" 2>/dev/null | head -5
echo ""
# Look for @rocket.chat/eslint-config in packages
fd "eslint-config" -t d | head -5
# Check the main eslint-config directory
if [ -d "packages/eslint-config" ]; then
echo "=== Contents of packages/eslint-config ==="
ls -la packages/eslint-config/
echo ""
echo "=== Main config file ==="
cat packages/eslint-config/index.js 2>/dev/null || cat packages/eslint-config/.eslintrc.json 2>/dev/null || echo "Not found"
fi
# Check packages/core-services ESLint configuration
echo -e "\n=== packages/core-services ESLint config ==="
if [ -d "packages/core-services" ]; then
cat packages/core-services/.eslintrc.json 2>/dev/null || echo ".eslintrc.json not found"
fi
# Check root .eslintrc.json for Jest overrides
echo -e "\n=== Root .eslintrc.json (first 50 lines) ==="
head -50 .eslintrc.json 2>/dev/null || echo "Not found"
# Check if spec files exist in core-services
echo -e "\n=== Check for spec files in packages/core-services ==="
find packages/core-services -name "*.spec.ts" -o -name "*.spec.js" 2>/dev/null | head -10Repository: RocketChat/Rocket.Chat
Length of output: 1589
🏁 Script executed:
#!/bin/bash
# Check the standard ESLint config content
echo "=== packages/eslint-config/standard/index.js ==="
cat packages/eslint-config/standard/index.js
# Check if other packages in monorepo have Jest spec files
echo -e "\n=== Packages with spec files in monorepo ==="
find packages ee/packages -name "*.spec.ts" -o -name "*.spec.js" 2>/dev/null | head -20
# Check ESLint config in one package that has spec files (if any)
echo -e "\n=== Checking for ESLint overrides in packages with tests ==="
for pkg in $(find packages ee/packages -maxdepth 2 -name ".eslintrc.json" | head -5); do
if grep -l "files.*spec" "$pkg" 2>/dev/null; then
echo "Found Jest override in: $pkg"
cat "$pkg"
fi
done
# Check if ee/packages/abac has Jest config
echo -e "\n=== ee/packages/abac/.eslintrc.json ==="
cat ee/packages/abac/.eslintrc.json 2>/dev/null || echo "Not found"Repository: RocketChat/Rocket.Chat
Length of output: 5328
The simplified lint commands are appropriate. However, note that @rocket.chat/eslint-config does not include Jest globals support by default. If test files are added to this package in the future, ensure Jest globals (describe, it, expect, etc.) are either imported from @jest/globals in test files or configure ESLint overrides with Jest support.
🤖 Prompt for AI Agents
In packages/core-services/package.json around lines 13 to 14, the lint scripts
are fine but the project ESLint config (@rocket.chat/eslint-config) doesn't
enable Jest globals; to prepare for future test files either (A) add an ESLint
override in the package's ESLint config enabling the jest environment or adding
the recommended plugin/config for Jest so describe/it/expect are recognized, or
(B) ensure test files import Jest globals from @jest/globals; implement one of
these approaches to avoid lint errors when adding tests.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #37925 +/- ##
===========================================
+ Coverage 71.70% 71.78% +0.08%
===========================================
Files 1453 1453
Lines 76689 76689
Branches 11138 11132 -6
===========================================
+ Hits 54988 55054 +66
+ Misses 21274 21206 -68
- Partials 427 429 +2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Proposed changes (including videos or screenshots)
This pull request simplifies and cleans up the configuration for the
core-servicespackage by removing unused Babel-related files and dependencies, updating ESLint and test script configurations, and streamlining the ESLint setup. The main focus is on modernizing and reducing unnecessary complexity in the development setup.Build and Tooling Cleanup:
babel.config.jsfile and all Babel-related dependencies frompackage.json, as Babel is no longer needed for this package. [1] [2]ESLint and Testing Improvements:
package.jsonscripts to lint all files by default and updated the fix command accordingly..eslintrc.json, relying on the default configuration instead.testscript topackage.jsonfor running Jest tests.Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.