feat: auto copy egg meta properties #3
Conversation
WalkthroughThis pull request introduces updates to the project's CI configuration, documentation, and source code. The changes primarily focus on expanding Node.js version support in GitHub Actions, updating dependencies, and adding Egg.js-related metadata to the package configuration. The modifications enhance the project's compatibility, documentation, and testing, with a particular emphasis on improving support for the Egg.js framework. Changes
Sequence DiagramsequenceDiagram
participant CI as GitHub Actions
participant Build as Build Process
participant Package as dist/package.json
participant Test as Test Suite
CI->>Build: Trigger build with multiple Node.js versions
Build->>Package: Copy metadata
Build->>Package: Add Egg.js properties
Package->>Test: Verify package configuration
Test-->>CI: Report test results
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 (
|
|
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: npm/@types/node@20.17.10, npm/tshy@2.0.1 |
commit: |
[skip ci] ## [1.3.0](v1.2.0...v1.3.0) (2024-12-22) ### Features * auto copy egg meta properties ([#3](#3)) ([8b1be69](8b1be69))
![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 (4)
test/fixtures/demo/package-init.json (1)
35-37: Document the egg metadata purposeConsider adding documentation to explain the purpose and impact of setting
"framework": truein the egg configuration.Would you like me to help draft documentation for these new egg metadata properties?
test/index.test.ts (1)
60-69: Add test cases for missing egg meta propertiesThe current test only covers the happy path where both
eggPluginandeggproperties exist. Consider adding test cases for:
- Missing eggPlugin property
- Missing egg property
- Empty/invalid property values
Example test cases:
it('should handle missing egg meta properties', async () => { // Setup package.json without egg meta properties const pkgWithoutEgg = { ...pkg }; delete pkgWithoutEgg.eggPlugin; delete pkgWithoutEgg.egg; fs.writeFileSync(packageFile, JSON.stringify(pkgWithoutEgg)); // Run the build await coffee.spawn('tshy', { cwd }) .expect('code', 0) .end(); // Verify dist/package.json doesn't contain the properties const distPkg = JSON.parse(fs.readFileSync(path.join(cwd, 'dist/package.json'), 'utf-8')); assert.equal(distPkg.eggPlugin, undefined); assert.equal(distPkg.egg, undefined); });README.md (2)
42-44: Enhance egg meta properties documentationThe current documentation briefly mentions the feature but lacks examples and structure details. Consider adding:
- Example package.json with egg meta properties
- Description of property structure and purpose
- Use cases or scenarios where this is useful
Example addition:
## Auto-copy egg meta properties Copies `eggPlugin` and `egg` properties from package.json to dist/package.json. This is useful for Egg.js plugins and frameworks. Example package.json: ```json { "eggPlugin": { "name": "your-plugin", "exports": { "import": "./dist/esm", "require": "./dist/commonjs" } }, "egg": { "framework": true } }🧰 Tools
🪛 LanguageTool
[uncategorized] ~42-~42: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...eURLToPath(import.meta.url)); } ``` ## Auto copy egg meta properties package.json Copy ...(AUTO_HYPHEN)
12-12: Fix hyphenation in headingsAdd hyphens to compound modifiers:
- "Auto set" → "Auto-set"
- "Auto copy" → "Auto-copy"
Also applies to: 42-42
🧰 Tools
🪛 LanguageTool
[uncategorized] ~12-~12: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...: https://npmjs.org/package/tshy-after Auto set package.json after [tshy](https://githu...(AUTO_HYPHEN)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/nodejs.yml(1 hunks)README.md(2 hunks)package.json(1 hunks)src/index.ts(1 hunks)test/fixtures/demo/package-init.json(1 hunks)test/index.test.ts(1 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[uncategorized] ~12-~12: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...: https://npmjs.org/package/tshy-after Auto set package.json after [tshy](https://githu...
(AUTO_HYPHEN)
[uncategorized] ~42-~42: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...eURLToPath(import.meta.url)); } ``` ## Auto copy egg meta properties package.json Copy ...
(AUTO_HYPHEN)
🔇 Additional comments (3)
.github/workflows/nodejs.yml (1)
15-15: LGTM! Verify Node.js 23 compatibility
Adding Node.js 23 to the test matrix improves coverage. However, let's verify there are no compatibility warnings.
✅ Verification successful
Node.js 23 compatibility verified
The codebase uses stable Node.js features:
- Standard
node:protocol imports for core modules import.meta.urlwith proper fallback for CJS- No experimental features or deprecated APIs detected
- Dev dependencies including
@types/node@^22.10.2are compatible
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any Node.js compatibility warnings in the codebase
rg -i "deprecated|warning|node.*compatibility"
Length of output: 48
Script:
#!/bin/bash
# Check package.json for Node.js version requirements and dependencies
cat package.json | jq '.engines, .dependencies, .devDependencies'
# Look for any specific Node.js version requirements in README
fd README.md --exec cat {}
Length of output: 1780
Script:
#!/bin/bash
# Check for any import.meta.url usage which might need Node.js version compatibility checks
ast-grep --pattern 'import.meta.url'
# Check for any experimental features or Node.js specific APIs
rg -i "experimental|--experimental|node:|node-api"
Length of output: 713
package.json (1)
30-30: Verify breaking changes in dependency updates
The major version updates warrant careful review:
@types/node: ^20.6.2 → ^22.10.2tshy: ^2.0.0 → ^3.0.2
Also applies to: 33-33
test/fixtures/demo/package-init.json (1)
28-34: Verify the egg-mock plugin name
The eggPlugin configuration uses "egg-mock" as the plugin name. Please verify if this is the intended name for this package.
✅ Verification successful
The initial results show "egg-mock" is used in the test file and fixture. Let's examine the test file and other package configurations to better understand if this is intentionally used as a test case.
"egg-mock" is intentionally used as a test fixture name
The test file confirms that "egg-mock" is deliberately used in test assertions to verify the copying of egg metadata properties. This is a test fixture specifically designed to verify the functionality of copying egg-related metadata, and the plugin name "egg-mock" is part of the test case expectations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for egg-mock references in the codebase
rg "egg-mock"
# Check if this is a test fixture by looking for test files
fd -t f "test" -e ts -e js
Length of output: 165
Script:
#!/bin/bash
# Check the content of the test file to understand the context
cat test/index.test.ts
# Check if there are any other package.json files in fixtures
fd package.json test/fixtures
Length of output: 2511
| eggPlugin: pkg.eggPlugin, | ||
| egg: pkg.egg, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for egg meta properties
The code directly copies eggPlugin and egg properties without checking if they exist in the source package.json. This could lead to undefined properties being copied to dist/package.json.
Consider adding validation:
writeFileSync('dist/package.json', JSON.stringify({
name: pkg.name,
version: pkg.version,
- eggPlugin: pkg.eggPlugin,
- egg: pkg.egg,
+ ...(pkg.eggPlugin && { eggPlugin: pkg.eggPlugin }),
+ ...(pkg.egg && { egg: pkg.egg }),
}, null, 2) + '\n')📝 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.
| eggPlugin: pkg.eggPlugin, | |
| egg: pkg.egg, | |
| ...(pkg.eggPlugin && { eggPlugin: pkg.eggPlugin }), | |
| ...(pkg.egg && { egg: pkg.egg }), |
Summary by CodeRabbit
New Features
eggPluginandeggproperties to the output package metadata.Bug Fixes
Chores