Skip to content
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.

Sign up for GitHub

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

fix: addresses issue and test for the issue succeeds #51

Merged
merged 1 commit into from
Jan 10, 2025
Merged

fix: addresses issue and test for the issue succeeds #51

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion packages/eslint-plugin-rxjs/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,8 @@
"exports": {
".": {
"require": "./index.cjs",
"types": "./index.d.ts"
"types": "./index.d.ts",
"import": "./index.cjs"
Comment on lines +34 to +35
Copy link

@coderabbitai coderabbitai bot Jan 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Critical: Entry point file index.cjs is missing

The package configuration points to index.cjs for both CommonJS and ESM imports, but this file doesn't exist in the package. This will cause the package to fail in both CommonJS and ESM environments.

  • Ensure the build process correctly generates index.cjs in the package root
  • Verify the build output structure matches the package.json configuration
🔗 Analysis chain

Verify ESM compatibility with .cjs extension

While adding the "import" condition is a step towards ESM support, pointing it to a .cjs file might cause issues in strict ESM environments. Consider:

  1. Creating a dedicated ESM entry point (index.mjs)
  2. Using dynamic imports for ESM compatibility

Let's verify ESM compatibility:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if the package works in ESM context
cd tests/esm-js
# Attempt ESM import
echo 'import pkg from "@smarttools/eslint-plugin-rxjs"; console.log(pkg);' > test.mjs
node --experimental-vm-modules test.mjs 2>&1 || true

Length of output: 941

Script:

#!/bin/bash
# Check package configuration and structure
echo "=== Package.json content ==="
cat packages/eslint-plugin-rxjs/package.json

echo -e "\n=== Index.cjs content ==="
cat packages/eslint-plugin-rxjs/index.cjs 2>/dev/null || echo "File not found"

echo -e "\n=== Check for MJS files ==="
fd -e mjs . packages/eslint-plugin-rxjs/

echo -e "\n=== Check for JS files ==="
fd -e js . packages/eslint-plugin-rxjs/ --exclude '*.cjs' --exclude '*.mjs'

Length of output: 1356

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that file is created as part of the build process and is not expected here

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DaveMBush Understood, thank you for the clarification. I'll note this for future reviews.

✏️ Learnings added 
Learnt from: DaveMBush
PR: DaveMBush/eslint-plugin-rxjs#51
File: packages/eslint-plugin-rxjs/package.json:34-35
Timestamp: 2025-01-10T15:20:25.714Z
Learning: In the @smarttools/eslint-plugin-rxjs package, index.cjs is generated during the build process and is not expected to exist in the source code.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!

}
}
}
8 changes: 8 additions & 0 deletions test/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"name": "eslint-rxjs-test",
"private": true,
"type": "module",
"dependencies": {
"@smarttools/eslint-plugin-rxjs": "link:../dist/packages/eslint-plugin-rxjs"
}
}
3 changes: 3 additions & 0 deletions tests/esm-js/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import eslintPluginRxjs from '@smarttools/eslint-plugin-rxjs';

console.log(eslintPluginRxjs);
3 changes: 3 additions & 0 deletions tests/esm-js/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"type": "module"
}
46 changes: 46 additions & 0 deletions tests/esm-js/test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
#!/bin/bash

# Exit on any 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 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


Loading