Dmb/circular dependency error when using recommended/49 #59
Conversation
…cy-error-when-using-recommended/49
|
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
packages/eslint-plugin-rxjs/src/index.tsOops! Something went wrong! :( ESLint: 9.18.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by WalkthroughThis pull request introduces significant changes to the ESLint RxJS plugin configuration and testing infrastructure. The modifications include restructuring the plugin's configuration in Changes
Possibly related PRs
Poem
Finishing Touches
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 (
|
|
View your CI Pipeline Execution ↗ for commit e02f5ca.
☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (12)
tests/circular-reference/index.js (2)
4-4: Fix typo in variable assignment.There's a missing space after the equals sign.
-const result =util.inspect(rxjs, { depth: null }); +const result = util.inspect(rxjs, { depth: null });
4-9: Consider improving circular reference detection robustness.The current implementation has two potential issues:
- Using
depth: nullcould be memory intensive for large objects- String matching on '[Circular' is fragile and might break with different Node.js versions
Consider using a recursive function to detect circular references or a more robust circular reference detection library.
tests/using-recommended/eslint.config.js (1)
7-11: Consider adding explicit rules configuration.While using the recommended config is good, consider adding explicit rules configuration for better maintainability and clarity.
{ files: ['**/*.ts'], plugins: { rxjs, }, + rules: { + // Add explicit rules here + }, },tests/extends-from/eslint.config.js (1)
30-30: Use path.join consistently for cross-platform compatibility.While path.join is used here, ensure all path references are using it for cross-platform compatibility.
tests/extends-from/base.eslint.config.js (2)
10-12: Document complex regex patterns.The regex pattern for name exceptions is complex and would benefit from documentation explaining the rationale for each excluded pattern.
names: { + // Exclude common Angular lifecycle hooks and store-related methods '^(canActivate|canActivateChild|canDeactivate|canLoad|intercept|resolve|validate|store|_store)$': false, },
4-40: Standardize rule severity levels.The configuration mixes 'warn' and 'off' severity levels. Consider standardizing the severity levels based on the rule's importance to code quality.
packages/eslint-plugin-rxjs/src/index.ts (1)
109-117: Consider adding JSDoc comments for the recommended config.While the configuration structure is good, adding JSDoc comments would help document the purpose and usage of this configuration object.
+/** + * Recommended configuration for the RxJS ESLint plugin. + * This configuration includes all recommended rules and their settings. + */ const recommendedConfig = {tests/circular-reference/test.sh (1)
10-13: Consider using mktemp for temporary directories.Using node_modules directly for testing could interfere with actual dependencies.
-DEST_DIR="$ROOT_DIR/node_modules/@smarttools/eslint-plugin-rxjs" +TEMP_DIR=$(mktemp -d) +DEST_DIR="$TEMP_DIR/node_modules/@smarttools/eslint-plugin-rxjs"tests/test.sh (4)
1-14: Enhance script robustness with additional safety measures.Consider adding these bash safety options to prevent common shell script pitfalls:
#!/bin/bash -# Exit on error -set -e +# Exit on error, undefined vars, pipe failures, and enable debug mode +set -euo pipefail + +# Debug mode if requested +if [[ "${DEBUG:-}" == "true" ]]; then + set -x +fiAlso, consider making TOTAL_TESTS dynamic:
-TOTAL_TESTS=4 +# Auto-calculate total tests +TOTAL_TESTS=$(find "$SCRIPT_DIR" -name "test.sh" -not -path "$SCRIPT_DIR/test.sh" | wc -l)
31-35: Add test script validation and parallel execution option.Consider adding existence checks for test scripts and an option for parallel execution to improve performance.
+# Function to validate test script +function validate_test_script() { + local test_path=$1 + if [[ ! -f "$test_path" ]]; then + echo "❌ Error: Test script not found: $test_path" + exit 1 + fi + if [[ ! -x "$test_path" ]]; then + echo "❌ Error: Test script not executable: $test_path" + exit 1 + fi +} + +# Validate all test scripts first +for test_path in \ + "$SCRIPT_DIR/esm-js/test.sh" \ + "$SCRIPT_DIR/using-recommended/test.sh" \ + "$SCRIPT_DIR/extends-from/test.sh" \ + "$SCRIPT_DIR/circular-reference/test.sh" +do + validate_test_script "$test_path" +done + +# Option to run tests in parallel +PARALLEL_EXECUTION=${PARALLEL_EXECUTION:-false} +if [[ "$PARALLEL_EXECUTION" == "true" ]]; then + echo "Running tests in parallel..." + { + run_test "ESM JS Tests" "$SCRIPT_DIR/esm-js/test.sh" & + run_test "Using Recommended Tests" "$SCRIPT_DIR/using-recommended/test.sh" & + run_test "Extends From Tests" "$SCRIPT_DIR/extends-from/test.sh" & + run_test "Circular Reference Tests" "$SCRIPT_DIR/circular-reference/test.sh" & + wait + } +else
37-53: Enhance test reporting with timing and statistics.Add execution time tracking and more detailed statistics to the test report.
+# Add at the beginning of the script +START_TIME=$(date +%s) + # Print summary report echo -e "\n📊 Test Summary Report" echo "=======================" +END_TIME=$(date +%s) +DURATION=$((END_TIME - START_TIME)) +echo "Duration: ${DURATION}s" echo "Total Tests: $TOTAL_TESTS" echo "Passed: $PASSED_TESTS" echo "Failed: $FAILED_TESTS" +echo "Success Rate: $(( (PASSED_TESTS * 100) / TOTAL_TESTS ))%" if [ ${#FAILED_NAMES[@]} -ne 0 ]; then echo -e "\n❌ Failed Tests:" for name in "${FAILED_NAMES[@]}"; do echo "- $name" done + echo -e "\nℹ️ Tip: Run with DEBUG=true for verbose output" exit 1 fi
1-53: Add signal handling for graceful cleanup.Consider adding signal handling to ensure proper cleanup when the script is interrupted.
+# Add after the script setup +# Cleanup function +cleanup() { + # Kill all running test processes + jobs -p | xargs -r kill + # Remove any temporary files + rm -f /tmp/test-output-* + exit 1 +} + +# Set up signal handling +trap cleanup SIGINT SIGTERM
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
packages/eslint-plugin-rxjs/src/index.ts(1 hunks)packages/eslint-plugin-rxjs/src/lib/configs/recommended.ts(0 hunks)test/package.json(0 hunks)tests/README.md(1 hunks)tests/circular-reference/index.js(1 hunks)tests/circular-reference/test.sh(1 hunks)tests/circular-reference/tsconfig.json(1 hunks)tests/extends-from/base.eslint.config.js(1 hunks)tests/extends-from/eslint.config.js(1 hunks)tests/extends-from/package.json(1 hunks)tests/extends-from/sample.ts(1 hunks)tests/extends-from/test.sh(1 hunks)tests/extends-from/tsconfig.json(1 hunks)tests/test.sh(1 hunks)tests/using-recommended/eslint.config.js(1 hunks)tests/using-recommended/package.json(1 hunks)tests/using-recommended/sample.ts(1 hunks)tests/using-recommended/test.sh(1 hunks)tests/using-recommended/tsconfig.json(1 hunks)
💤 Files with no reviewable changes (2)
- test/package.json
- packages/eslint-plugin-rxjs/src/lib/configs/recommended.ts
✅ Files skipped from review due to trivial changes (8)
- tests/README.md
- tests/using-recommended/tsconfig.json
- tests/circular-reference/tsconfig.json
- tests/extends-from/tsconfig.json
- tests/using-recommended/sample.ts
- tests/using-recommended/package.json
- tests/extends-from/sample.ts
- tests/extends-from/package.json
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: main
🔇 Additional comments (7)
tests/using-recommended/eslint.config.js (1)
4-21: Verify typescript-eslint parser compatibility.The configuration looks good, but please ensure the typescript-eslint parser version is compatible with the flat config format. Recent versions of typescript-eslint have specific requirements for the new flat config system.
✅ Verification successful
TypeScript parser is fully compatible with flat config
The configuration is using @typescript-eslint/parser v8.20.0 which has full support for ESLint's flat config format. The syntax and structure of your configuration is correct.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check typescript-eslint parser version compatibility npm list @typescript-eslint/parser | grep parserLength of output: 65871
tests/extends-from/eslint.config.js (1)
24-36: Consider explicit configuration over spread operators.The current use of spread operators with config mapping could mask configuration issues. Consider being more explicit about which properties are being merged.
tests/extends-from/base.eslint.config.js (1)
2-2: Consider migrating away from recommended-legacy.Using the legacy configuration might be contributing to the circular dependency issue. Consider migrating to the new recommended configuration if possible.
packages/eslint-plugin-rxjs/src/index.ts (2)
104-107: LGTM! Good separation of base plugin configuration.Clean separation of base plugin configuration helps prevent circular dependencies.
119-125: Verify the impact of configuration changes.The restructuring of configurations looks good, but let's verify that existing projects using the plugin won't be affected by these changes.
✅ Verification successful
Configuration changes maintain backward compatibility ✅
The changes preserve both
recommendedandrecommended-legacyconfigurations, ensuring existing projects will continue to work. The test configuration demonstrates successful usage of the legacy config, and no breaking changes were identified.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for projects extending or using the recommended configurations rg -l "(@smarttools/rxjs/recommended|@smarttools/rxjs/recommended-legacy)" -g "*.{js,json}"Length of output: 134
Script:
#!/bin/bash # Check the content of the found test file echo "=== Content of test configuration file ===" cat tests/extends-from/base.eslint.config.js # Search for alternative patterns echo -e "\n=== Alternative config usage patterns ===" rg -l "(plugin:@smarttools/rxjs|extends.*rxjs)" -g "*.{js,json}" # Check the configuration objects echo -e "\n=== Configuration objects ===" ast-grep --pattern 'const recommendedConfig = $_' ast-grep --pattern 'const recommended = $_'Length of output: 2092
tests/circular-reference/test.sh (1)
1-9: LGTM! Good error handling setup.The script properly sets up error handling and directory resolution.
tests/extends-from/test.sh (1)
1-42: Apply the same shared utilities pattern here.This script would benefit from the same shared utilities refactoring suggested for the using-recommended test.
| # Run the index.js file | ||
| echo "Running index.js..." | ||
| if node "$SCRIPT_DIR/index.js"; then | ||
| echo "✓ Tests completed successfully" | ||
| exit 0 | ||
| else | ||
| echo "✗ Tests failed" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add cleanup for temporary resources.
The script should clean up resources even if tests fail.
+# Cleanup function
+cleanup() {
+ rm -rf "$DEST_DIR"
+ echo "Cleaned up test resources"
+}
+
+# Set up trap for cleanup
+trap cleanup EXIT
+
# Run the index.js file
echo "Running index.js..."
if node "$SCRIPT_DIR/index.js"; then
echo "✓ Tests completed successfully"
- exit 0
+ exit_code=0
else
echo "✗ Tests failed"
- exit 1
+ exit_code=1
fi
+
+exit $exit_code📝 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.
| # Run the index.js file | |
| echo "Running index.js..." | |
| if node "$SCRIPT_DIR/index.js"; then | |
| echo "✓ Tests completed successfully" | |
| exit 0 | |
| else | |
| echo "✗ Tests failed" | |
| exit 1 | |
| fi | |
| # Cleanup function | |
| cleanup() { | |
| rm -rf "$DEST_DIR" | |
| echo "Cleaned up test resources" | |
| } | |
| # Set up trap for cleanup | |
| trap cleanup EXIT | |
| # Run the index.js file | |
| echo "Running index.js..." | |
| if node "$SCRIPT_DIR/index.js"; then | |
| echo "✓ Tests completed successfully" | |
| exit_code=0 | |
| else | |
| echo "✗ Tests failed" | |
| exit_code=1 | |
| fi | |
| exit $exit_code |
There was a problem hiding this comment.
I've created a followup task for this
There was a problem hiding this comment.
@DaveMBush Thanks for creating a follow-up task! That's a great way to ensure the cleanup functionality gets implemented.
⟦ ∙ᴥ∙ ⟧ happy to help
| #!/bin/bash | ||
|
|
||
| # Exit on error | ||
| set -e | ||
|
|
||
| # Get the directory where the script is located | ||
| SCRIPT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )" | ||
| ROOT_DIR="$SCRIPT_DIR/../.." | ||
|
|
||
| # Define source and destination paths relative to root | ||
| SRC_DIR="$ROOT_DIR/dist/packages/eslint-plugin-rxjs" | ||
| DEST_DIR="$ROOT_DIR/node_modules/@smarttools/eslint-plugin-rxjs" | ||
|
|
||
| # Check if source directory exists | ||
| if [ ! -d "$SRC_DIR" ]; then | ||
| echo "Error: Source directory $SRC_DIR does not exist" | ||
| exit 1 | ||
| fi | ||
|
|
||
| # Create destination parent directory if it doesn't exist | ||
| mkdir -p "$(dirname "$DEST_DIR")" | ||
|
|
||
| # Remove destination directory if it exists | ||
| if [ -d "$DEST_DIR" ]; then | ||
| rm -rf "$DEST_DIR" | ||
| fi | ||
|
|
||
| # Copy the directory | ||
| if cp -r "$SRC_DIR" "$DEST_DIR"; then | ||
| echo "✓ Successfully copied $SRC_DIR to $DEST_DIR" | ||
| else | ||
| echo "✗ Failed to copy directory" | ||
| exit 1 | ||
| fi | ||
|
|
||
| # Run ESLint with the specific config and test file | ||
| if pnpm eslint --config $SCRIPT_DIR/eslint.config.js $SCRIPT_DIR/sample.ts; then | ||
| echo -e "\n✅ Linting PASSED" | ||
| else | ||
| echo -e "\n❌ Linting FAILED" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider creating a shared test utilities script.
This script shares significant code with other test scripts. Consider extracting common functionality into a shared utilities script.
Create a new file tests/utils.sh:
#!/bin/bash
setup_test_env() {
local root_dir="$1"
local script_dir="$2"
# Create temp directory
local temp_dir=$(mktemp -d)
local dest_dir="$temp_dir/node_modules/@smarttools/eslint-plugin-rxjs"
local src_dir="$root_dir/dist/packages/eslint-plugin-rxjs"
# Copy plugin to test location
mkdir -p "$(dirname "$dest_dir")"
cp -r "$src_dir" "$dest_dir" || {
echo "✗ Failed to copy directory"
rm -rf "$temp_dir"
return 1
}
echo "✓ Successfully copied $src_dir to $dest_dir"
echo "$temp_dir"
}
cleanup_test_env() {
local temp_dir="$1"
rm -rf "$temp_dir"
echo "Cleaned up test resources"
}Then modify this script to:
#!/bin/bash
set -e
SCRIPT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"
ROOT_DIR="$SCRIPT_DIR/../.."
# Source common utilities
source "$ROOT_DIR/tests/utils.sh"
# Setup test environment
TEMP_DIR=$(setup_test_env "$ROOT_DIR" "$SCRIPT_DIR")
trap 'cleanup_test_env "$TEMP_DIR"' EXIT
# Run ESLint with the specific config and test file
if pnpm eslint --config "$SCRIPT_DIR/eslint.config.js" "$SCRIPT_DIR/sample.ts"; then
echo -e "\n✅ Linting PASSED"
exit 0
else
echo -e "\n❌ Linting FAILED"
exit 1
fiThere was a problem hiding this comment.
I've created a followup task for this
There was a problem hiding this comment.
@DaveMBush Thanks for creating a follow-up task! Let me know if you need any assistance when you get to implementing the shared utilities.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| # Function to run a test and track its result | ||
| function run_test() { | ||
| local test_name=$1 | ||
| local test_path=$2 | ||
|
|
||
| echo -e "\n📋 Running test: $test_name" | ||
| if bash "$test_path"; then | ||
| echo "✅ $test_name passed" | ||
| PASSED_TESTS=$((PASSED_TESTS + 1)) | ||
| else | ||
| echo "❌ $test_name failed" | ||
| FAILED_TESTS=$((FAILED_TESTS + 1)) | ||
| FAILED_NAMES+=("$test_name") | ||
| fi | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test runner with timeout and error handling.
The current implementation could benefit from additional error handling and debugging capabilities.
function run_test() {
local test_name=$1
local test_path=$2
+ local timeout_seconds=300 # 5 minutes timeout
+ local output_file
+ output_file=$(mktemp)
echo -e "\n📋 Running test: $test_name"
- if bash "$test_path"; then
+ if timeout "$timeout_seconds" bash "$test_path" > "$output_file" 2>&1; then
echo "✅ $test_name passed"
PASSED_TESTS=$((PASSED_TESTS + 1))
else
- echo "❌ $test_name failed"
+ local exit_code=$?
+ echo "❌ $test_name failed with exit code $exit_code"
+ echo "Last 10 lines of output:"
+ tail -n 10 "$output_file"
FAILED_TESTS=$((FAILED_TESTS + 1))
FAILED_NAMES+=("$test_name")
fi
+ rm -f "$output_file"
}📝 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.
| # Function to run a test and track its result | |
| function run_test() { | |
| local test_name=$1 | |
| local test_path=$2 | |
| echo -e "\n📋 Running test: $test_name" | |
| if bash "$test_path"; then | |
| echo "✅ $test_name passed" | |
| PASSED_TESTS=$((PASSED_TESTS + 1)) | |
| else | |
| echo "❌ $test_name failed" | |
| FAILED_TESTS=$((FAILED_TESTS + 1)) | |
| FAILED_NAMES+=("$test_name") | |
| fi | |
| } | |
| # Function to run a test and track its result | |
| function run_test() { | |
| local test_name=$1 | |
| local test_path=$2 | |
| local timeout_seconds=300 # 5 minutes timeout | |
| local output_file | |
| output_file=$(mktemp) | |
| echo -e "\n📋 Running test: $test_name" | |
| if timeout "$timeout_seconds" bash "$test_path" > "$output_file" 2>&1; then | |
| echo "✅ $test_name passed" | |
| PASSED_TESTS=$((PASSED_TESTS + 1)) | |
| else | |
| local exit_code=$? | |
| echo "❌ $test_name failed with exit code $exit_code" | |
| echo "Last 10 lines of output:" | |
| tail -n 10 "$output_file" | |
| FAILED_TESTS=$((FAILED_TESTS + 1)) | |
| FAILED_NAMES+=("$test_name") | |
| fi | |
| rm -f "$output_file" | |
| } |
There was a problem hiding this comment.
I've created a followup task for this
There was a problem hiding this comment.
@DaveMBush Thanks for creating a follow-up task! Let me know if you need any assistance when you get to implementing those enhancements.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
DaveMBush
deleted the
dmb/circular-dependency-error-when-using-recommended/49
branch
Issue Number: #49 and #48
Body
Adds tests for scenarios.
Removes name prop on recommended-legacy
Addresses circular reference
Summary by CodeRabbit
Release Notes
New Features
Testing
Configuration
Chores