fix(semantic-release-clean-package-json): reworked this plugin to use… #112
Conversation
WalkthroughThis pull request adds the Changes
Sequence DiagramsequenceDiagram
participant Plugin as Clean Package JSON Plugin
participant SemanticRelease as Semantic Release
participant PackageJSON as package.json
SemanticRelease->>Plugin: Trigger publish
Plugin->>PackageJSON: Create backup
Plugin->>PackageJSON: Remove unnecessary properties
Plugin->>SemanticRelease: Publish completed
SemanticRelease->>Plugin: Trigger success
Plugin->>PackageJSON: Restore from backup
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (8)
✨ 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 (
|
|
Thank you for following the naming conventions! 🙏 |
|
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: npm/get-stream@9.0.1, npm/git-log-parser@1.2.1 |
prisis
force-pushed
the
changed-states-of-plugin
branch
from
9b8c952 to
a9abf56
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 3
🧹 Nitpick comments (4)
packages/semantic-release-clean-package-json/src/index.ts (1)
34-66: Refactor Loops for Better Performance and ReadabilityThe use of
for...inloops anddeletestatements can be inefficient and may lead to unintended behavior if prototype properties are inherited. Consider usingObject.keys()orObject.entries()withforEachto iterate over the object's own properties more safely and efficiently.Apply this diff to refactor the loops:
- for (const property in packageJsonCopy) { + Object.keys(packageJsonCopy).forEach((property) => { if (keepProperties.has(property)) { // eslint-disable-next-line no-continue return; } if (property === "scripts") { // eslint-disable-next-line no-loops/no-loops,no-restricted-syntax - for (const script in packageJsonCopy.scripts) { + Object.keys(packageJsonCopy.scripts).forEach((script) => { if (keepProperties.has(`${property}.${script}`)) { // eslint-disable-next-line no-continue return; } // eslint-disable-next-line @typescript-eslint/no-dynamic-delete,security/detect-object-injection delete packageJsonCopy.scripts[script]; - } + }); } if (packageJsonCopy.scripts && Object.keys(packageJsonCopy.scripts).length > 0) { // eslint-disable-next-line no-continue return; } context.logger.log(`Removing property "${property}"`); // eslint-disable-next-line @typescript-eslint/no-dynamic-delete,security/detect-object-injection delete packageJsonCopy[property]; - } + });packages/semantic-release-clean-package-json/__tests__/index.test.ts (2)
65-65: Correct the Test Description GrammarThe test description has a grammatical error. It should read "should remove unnecessary properties" to maintain proper grammar.
Apply this diff to fix the grammar:
- it("should removes unnecessary properties", async () => { + it("should remove unnecessary properties", async () => {
89-89: Clarify Test Description for Better UnderstandingThe test description could be more precise. Consider rephrasing it to clearly convey its purpose.
Apply this diff to improve clarity:
- it("should keep flag from given config", async () => { + it("should keep properties specified in the given config", async () => {packages/semantic-release-clean-package-json/README.md (1)
57-57: Consider rephrasing the importance notice.The current phrasing "Very important" is redundant with the
[!IMPORTANT]notice. Consider simplifying to:-> Very important: The plugin must be placed before... +> The plugin must be placed before...🧰 Tools
🪛 LanguageTool
[style] ~57-~57: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...on.md#configuration): > [!IMPORTANT] > Very important: The plugin must be placed before the `...(EN_WEAK_ADJECTIVE)
🛑 Comments failed to post (3)
packages/semantic-release-clean-package-json/src/index.ts (1)
15-15:
⚠️ Potential issueValidate
pkgRootto Prevent Directory Traversal VulnerabilitiesWhen resolving
pkgRoot, ensure that it does not lead outside of the intended directory structure. This validation prevents potential security issues where a maliciouspkgRootcould cause unintended file access or modification.Apply this diff to validate
pkgRoot:const cwd = pluginConfig.pkgRoot ? resolve(context.cwd, pluginConfig.pkgRoot) : context.cwd; + if (!cwd.startsWith(context.cwd)) { + throw new Error("Invalid pkgRoot path. It must be within the project directory."); + }📝 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.const cwd = pluginConfig.pkgRoot ? resolve(context.cwd, pluginConfig.pkgRoot) : context.cwd; if (!cwd.startsWith(context.cwd)) { throw new Error("Invalid pkgRoot path. It must be within the project directory."); }packages/semantic-release-clean-package-json/.releaserc.json (1)
30-31: 💡 Codebase verification
Build step required before the plugin can work
The plugin configuration is correct, but the
dist/index.mjsfile doesn't exist yet. The build setup is in place:
- ✓ Source file:
src/index.ts- ✓ Build script:
packem build --development- ✓ Dependency
@anolilab/semantic-release-pnpmis properly declaredMake sure to run
pnpm buildbefore using the plugin.🔗 Analysis chain
Plugin order looks correct.
The placement of
./dist/index.mjsand@anolilab/semantic-release-pnpmbefore the GitHub plugin aligns with the documented requirements, ensuring the cleaned package.json isn't published to GitHub.Let's verify the dist file exists and the dependency is properly declared:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify dist/index.mjs exists fd -t f "index.mjs" packages/semantic-release-clean-package-json/dist # Check if @anolilab/semantic-release-pnpm is declared as a dependency cat packages/semantic-release-clean-package-json/package.json | jq '.dependencies["@anolilab/semantic-release-pnpm"] // .devDependencies["@anolilab/semantic-release-pnpm"] // empty'Length of output: 406
Script:
#!/bin/bash # Check for build script and source file cat packages/semantic-release-clean-package-json/package.json | jq '.scripts.build' # Look for source file that might be built into dist/index.mjs fd -t f "index.ts" packages/semantic-release-clean-package-json/srcLength of output: 238
packages/semantic-release-preset/package.json (1)
3-3: 💡 Codebase verification
⚠️ Potential issueLock file is out of sync with package.json dependencies
The lock file contains outdated versions:
@anolilab/semantic-release-clean-package-json: 1.0.1-alpha.2 (needs update to 1.0.1-alpha.3)@anolilab/semantic-release-pnpm: Using workspace reference instead of explicit version 1.1.9-alpha.1Run
pnpm installto update the lock file with the new versions.🔗 Analysis chain
Coordinated version updates need lock file sync.
The version updates are consistently applied across:
- Package version:
10.0.1-alpha.3- Dependencies:
@anolilab/semantic-release-clean-package-json:1.0.1-alpha.3@anolilab/semantic-release-pnpm:1.1.9-alpha.1(both in devDependencies and peerDependencies)However, the pipeline failures indicate that these updates are not reflected in the lock file.
Also applies to: 89-89, 101-101, 119-119
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify version consistency in lock file echo "Checking semantic-release-clean-package-json version..." rg -A 1 '"@anolilab/semantic-release-clean-package-json":\s*"1\.0\.1-alpha\.3"' pnpm-lock.yaml || echo "Version mismatch for semantic-release-clean-package-json" echo "Checking semantic-release-pnpm version..." rg -A 1 '"@anolilab/semantic-release-pnpm":\s*"1\.1\.9-alpha\.1"' pnpm-lock.yaml || echo "Version mismatch for semantic-release-pnpm"Length of output: 611
Script:
#!/bin/bash # Find actual versions in lock file echo "Current versions in lock file for semantic-release-clean-package-json:" rg "@anolilab/semantic-release-clean-package-json.*version:" pnpm-lock.yaml echo -e "\nCurrent versions in lock file for semantic-release-pnpm:" rg "@anolilab/semantic-release-pnpm.*version:" pnpm-lock.yamlLength of output: 421
Script:
#!/bin/bash # Broader search for packages in lock file echo "Searching for semantic-release-clean-package-json in lock file:" rg "@anolilab/semantic-release-clean-package-json" pnpm-lock.yaml echo -e "\nSearching for semantic-release-pnpm in lock file:" rg "@anolilab/semantic-release-pnpm" pnpm-lock.yaml echo -e "\nLock file structure check:" head -n 5 pnpm-lock.yamlLength of output: 971
🧰 Tools
🪛 GitHub Actions: Preview Release
[error] Lock file is out of sync with package.json. Package versions in pnpm-lock.yaml do not match the specifications in package.json. Notable difference: @anolilab/semantic-release-clean-package-json version mismatch (1.0.1-alpha.2 in lock file vs 1.0.1-alpha.3 in package.json)
🪛 GitHub Actions: Lint
[error] Lock file is out of sync with package.json. Package versions in pnpm-lock.yaml do not match the specifications in package.json. Notable difference: @anolilab/semantic-release-clean-package-json version mismatch (1.0.1-alpha.2 in lock file vs 1.0.1-alpha.3 in package.json)
commit: |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #112 +/- ##
===========================================
+ Coverage 62.40% 84.21% +21.80%
===========================================
Files 32 8 -24
Lines 758 190 -568
Branches 118 23 -95
===========================================
- Hits 473 160 -313
+ Misses 275 24 -251
+ Partials 10 6 -4
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
packages/semantic-release-clean-package-json/README.md (1)
57-57: Enhance the warning message clarity.The breaking change regarding plugin order is well-documented. However, consider refining the warning message for better clarity.
-> Very important: The plugin must be placed before the `@semantic-release/github` or `@semantic-release/git` and before `@anolilab/semantic-release-pnpm` or `@semantic-release/npm` plugin otherwise the `package.json` will be cleaned and published into GitHub / Your Git Provider. +> Critical: Place this plugin before `@semantic-release/github`, `@semantic-release/git`, `@anolilab/semantic-release-pnpm`, and `@semantic-release/npm` plugins to prevent publishing a cleaned package.json to your Git provider.Also applies to: 65-66
🧰 Tools
🪛 LanguageTool
[style] ~57-~57: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...on.md#configuration): > [!IMPORTANT] > Very important: The plugin must be placed before the `...(EN_WEAK_ADJECTIVE)
packages/semantic-release-clean-package-json/src/index.ts (3)
1-11: LGTM! Consider grouping related imports.The imports appropriately support the new backup/restore functionality. Consider grouping related imports (node built-ins, external packages, types, local imports) for better organization.
import { rm } from "node:fs/promises"; import { isAccessible, readJson, writeJson } from "@visulima/fs"; import { join, resolve } from "@visulima/path"; + import type { PackageJson } from "type-fest"; + import defaultKeepProperties from "./default-keep-properties"; import type { CommonContext, PublishContext } from "./definitions/context"; import type { PluginConfig } from "./definitions/plugin-config"; import getPackage from "./utils/get-pkg";
12-13: Consider refactoring to reduce cognitive complexity.The ESLint comment suggests high cognitive complexity. Consider breaking down the function into smaller, focused functions for better maintainability.
34-60: Consider using Object.entries for safer property iteration.The current implementation uses for...in which can iterate over inherited properties. Using Object.entries would be safer and more idiomatic.
- for (const property in packageJsonCopy) { + for (const [property, value] of Object.entries(packageJsonCopy)) { if (keepProperties.has(property)) { continue; } if (property === "scripts") { - for (const script in packageJsonCopy.scripts) { + for (const [script] of Object.entries(packageJsonCopy.scripts ?? {})) { if (keepProperties.has(`${property}.${script}`)) { continue; } delete packageJsonCopy.scripts[script]; } if (packageJsonCopy.scripts && Object.keys(packageJsonCopy.scripts).length > 0) { continue; } } context.logger.log(`Removing property "${property}"`); delete packageJsonCopy[property]; }packages/semantic-release-clean-package-json/__tests__/index.test.ts (3)
7-8: LGTM! Consider adding type assertion helper.The test setup is well-structured. Consider creating a helper function for context type assertions to reduce repetitive casting.
const asPublishContext = (ctx: Partial<PublishContext>): PublishContext => ctx as PublishContext; const asCommonContext = (ctx: Partial<CommonContext>): CommonContext => ctx as CommonContext;Also applies to: 28-57
Line range hint
64-122: Consider adding error case tests.The happy path tests are thorough. Consider adding test cases for:
- Failed backup creation
- Invalid package.json content
- Write permission issues
Example test case:
it("should handle backup creation failure", async () => { const packageJsonPath = `${temporaryDirectoryPath}/package.json`; await writeJson(packageJsonPath, DEFAULT_PACKAGE_JSON); // Mock writeJson to fail vi.mocked(writeJson).mockRejectedValueOnce(new Error("Write failed")); await expect(publish({}, { cwd: temporaryDirectoryPath, ...context } as PublishContext)) .rejects.toThrow("Write failed"); expect((context as PublishContext).logger.error) .toHaveBeenCalledWith(expect.stringContaining("Failed to create backup")); });
124-182: Add edge case tests for success function.Consider adding test cases for:
- Corrupted backup file
- Version mismatch scenarios
- Partial backup content
Example test case:
it("should handle corrupted backup file", async () => { const packageJsonPath = `${temporaryDirectoryPath}/package.json`; const backupPath = `${temporaryDirectoryPath}/package.json.back`; // Create corrupted backup await writeJson(packageJsonPath, DEFAULT_PACKAGE_JSON); await writeJson(backupPath, "{ invalid json }"); await expect(success({}, { cwd: temporaryDirectoryPath, ...context } as CommonContext)) .rejects.toThrow(); expect((context as CommonContext).logger.error) .toHaveBeenCalledWith(expect.stringContaining("Failed to restore")); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
README.md(1 hunks)packages/semantic-release-clean-package-json/.releaserc.json(2 hunks)packages/semantic-release-clean-package-json/CHANGELOG.md(2 hunks)packages/semantic-release-clean-package-json/README.md(6 hunks)packages/semantic-release-clean-package-json/__tests__/index.test.ts(5 hunks)packages/semantic-release-clean-package-json/package.json(2 hunks)packages/semantic-release-clean-package-json/src/definitions/context.ts(1 hunks)packages/semantic-release-clean-package-json/src/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- README.md
- packages/semantic-release-clean-package-json/.releaserc.json
- packages/semantic-release-clean-package-json/CHANGELOG.md
- packages/semantic-release-clean-package-json/package.json
- packages/semantic-release-clean-package-json/src/definitions/context.ts
🧰 Additional context used
🪛 LanguageTool
packages/semantic-release-clean-package-json/README.md
[style] ~57-~57: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...on.md#configuration): > [!IMPORTANT] > Very important: The plugin must be placed before the `...
(EN_WEAK_ADJECTIVE)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: Test semantic-release version ^23 on (node-18, ubuntu-latest)
- GitHub Check: Test semantic-release version ^22 on (node-18, ubuntu-latest)
- GitHub Check: Test (node-22, ubuntu-latest)
- GitHub Check: Test semantic-release version ^21 on (node-18, ubuntu-latest)
- GitHub Check: Test (node-21, ubuntu-latest)
- GitHub Check: Test semantic-release version ^20 on (node-18, ubuntu-latest)
- GitHub Check: Test (node-20, ubuntu-latest)
- GitHub Check: Test (node-18, ubuntu-latest)
- GitHub Check: Lint (Typescript types)
- GitHub Check: Lint (package.json)
- GitHub Check: Lint (eslint)
- GitHub Check: Preview Release
🔇 Additional comments (4)
packages/semantic-release-clean-package-json/README.md (3)
4-4: Well-structured introduction and rationale!The new description and "Why?" section effectively communicate the plugin's purpose and benefits. The content is clear, informative, and helps users understand the value proposition.
Also applies to: 28-37
75-76: Excellent documentation of the new workflow steps!The steps table clearly documents the new
publishandsuccessworkflow, providing detailed information about each step's actions. This aligns perfectly with the breaking changes mentioned in the PR objectives.
Line range hint
91-150: Comprehensive and well-structured examples!The examples effectively demonstrate:
- Basic configuration with custom properties
- TypeScript-specific configuration
- Custom package root setup
The examples align with the new plugin ordering requirements and provide clear guidance for different use cases.
packages/semantic-release-clean-package-json/src/index.ts (1)
17-23: Verify error handling for writeJson operation.The backup creation is crucial for the restore functionality. Consider adding try-catch block to handle potential write failures.
+ try { await writeJson( join(cwd, "package.json.back"), packageJson, { detectIndent: true, }, ); + } catch (error) { + context.logger.error(`Failed to create backup: ${error.message}`); + throw error; + }✅ Verification successful
The suggested error handling is appropriate and aligns with existing patterns.
The backup creation is indeed critical for the restore functionality, and the suggested error handling:
- Uses the established context.logger pattern found elsewhere in the codebase
- Provides clear error messaging
- Properly propagates the error while ensuring it's logged
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Check for logger usage patterns rg "context\.logger\." -A 2 # Look for writeJson operations and their error handling rg "writeJson" -A 5 # Look for try-catch blocks to understand error handling patterns ast-grep --pattern 'try { $$$ } catch ($error) { $$$ }'Length of output: 15720
| export const success = async (pluginConfig: PluginConfig, context: CommonContext): Promise<void> => { | ||
| const cwd = pluginConfig.pkgRoot ? resolve(context.cwd, pluginConfig.pkgRoot) : context.cwd; | ||
|
|
||
| const backupPackageJson = join(cwd, "package.json.back"); | ||
|
|
||
| if (await isAccessible(backupPackageJson)) { | ||
| const packageJson = await getPackage(pluginConfig, context); | ||
|
|
||
| const backupPackageJsonContent = (await readJson(backupPackageJson)) as PackageJson; | ||
|
|
||
| // Overwrite the version from the backup package.json | ||
| backupPackageJsonContent.version = packageJson.version; | ||
|
|
||
| await writeJson( | ||
| join(cwd, "package.json"), | ||
| backupPackageJsonContent, | ||
| { | ||
| detectIndent: true, | ||
| overwrite: true, | ||
| }, | ||
| ); | ||
|
|
||
| await rm(backupPackageJson); | ||
|
|
||
| context.logger.log("Restored modified package.json from backup."); | ||
| } else { | ||
| context.logger.error("No backup package.json found."); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling in restore operation.
The success function should handle potential errors during file operations and provide more detailed error messages.
try {
if (await isAccessible(backupPackageJson)) {
const packageJson = await getPackage(pluginConfig, context);
const backupPackageJsonContent = (await readJson(backupPackageJson)) as PackageJson;
backupPackageJsonContent.version = packageJson.version;
await writeJson(
join(cwd, "package.json"),
backupPackageJsonContent,
{
detectIndent: true,
overwrite: true,
},
);
await rm(backupPackageJson);
context.logger.log("Restored modified package.json from backup.");
} else {
context.logger.error("No backup package.json found.");
}
+ } catch (error) {
+ context.logger.error(`Failed to restore package.json: ${error.message}`);
+ throw error;
}📝 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.
| export const success = async (pluginConfig: PluginConfig, context: CommonContext): Promise<void> => { | |
| const cwd = pluginConfig.pkgRoot ? resolve(context.cwd, pluginConfig.pkgRoot) : context.cwd; | |
| const backupPackageJson = join(cwd, "package.json.back"); | |
| if (await isAccessible(backupPackageJson)) { | |
| const packageJson = await getPackage(pluginConfig, context); | |
| const backupPackageJsonContent = (await readJson(backupPackageJson)) as PackageJson; | |
| // Overwrite the version from the backup package.json | |
| backupPackageJsonContent.version = packageJson.version; | |
| await writeJson( | |
| join(cwd, "package.json"), | |
| backupPackageJsonContent, | |
| { | |
| detectIndent: true, | |
| overwrite: true, | |
| }, | |
| ); | |
| await rm(backupPackageJson); | |
| context.logger.log("Restored modified package.json from backup."); | |
| } else { | |
| context.logger.error("No backup package.json found."); | |
| } | |
| }; | |
| export const success = async (pluginConfig: PluginConfig, context: CommonContext): Promise<void> => { | |
| const cwd = pluginConfig.pkgRoot ? resolve(context.cwd, pluginConfig.pkgRoot) : context.cwd; | |
| const backupPackageJson = join(cwd, "package.json.back"); | |
| try { | |
| if (await isAccessible(backupPackageJson)) { | |
| const packageJson = await getPackage(pluginConfig, context); | |
| const backupPackageJsonContent = (await readJson(backupPackageJson)) as PackageJson; | |
| // Overwrite the version from the backup package.json | |
| backupPackageJsonContent.version = packageJson.version; | |
| await writeJson( | |
| join(cwd, "package.json"), | |
| backupPackageJsonContent, | |
| { | |
| detectIndent: true, | |
| overwrite: true, | |
| }, | |
| ); | |
| await rm(backupPackageJson); | |
| context.logger.log("Restored modified package.json from backup."); | |
| } else { | |
| context.logger.error("No backup package.json found."); | |
| } | |
| } catch (error) { | |
| context.logger.error(`Failed to restore package.json: ${error.message}`); | |
| throw error; | |
| } | |
| }; |
… the `publish` and `success` functions, to have the correct state on git BREAKING-CHANGE: Changed plugin steps to `publish` and `success` from `prepare`
prisis
force-pushed
the
changed-states-of-plugin
branch
from
1d5d322 to
b0b8500
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
@coderabbitai summary |
✅ Actions performedSummary regeneration triggered. |
|
@coderabbitai summary |
✅ Actions performedSummary regeneration triggered. |
…skip ci]\n\n## @anolilab/semantic-release-clean-package-json [2.0.0](https://github.com/anolilab/semantic-release/compare/@anolilab/semantic-release-clean-package-json@1.0.0...@anolilab/semantic-release-clean-package-json@2.0.0) (2025-01-22) ### ⚠ BREAKING CHANGES * **semantic-release-clean-package-json:** Changed plugin steps to publish and success from prepare ### Bug Fixes * **semantic-release-clean-package-json:** reworked this plugin to use the `publish` and `success` functions, to have the correct state on git ([#112](#112)) ([734d48a](734d48a)) ### Dependencies * **@anolilab/semantic-release-pnpm:** upgraded to 1.1.9
… the
publishandsuccessfunctions, to have the correct state on gitBREAKING-CHANGE: Changed plugin steps to
publishandsuccessfromprepareSummary by CodeRabbit
New Features
Documentation