-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Fix publish-npm script #7093
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
Fix publish-npm script #7093
Conversation
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.
Thank you for this bug fix! The change correctly addresses the issue by updating the path from packages/sdk/npm/package to packages/types/npm/package. I've reviewed the changes and have some suggestions for future improvements.
| try { | ||
| const status = execSync("git status --porcelain", { encoding: "utf8" }) | ||
| const relevantChanges = status.split("\n").filter((line) => line.includes("packages/sdk/npm/package")) | ||
| const relevantChanges = status.split("\n").filter((line) => line.includes("packages/types/npm/package")) |
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.
Good fix! For future improvements, consider adding a validation check to ensure the path exists before filtering. This would make the script more robust:
| const relevantChanges = status.split("\n").filter((line) => line.includes("packages/types/npm/package")) | |
| const relevantChanges = status.split(" | |
| ").filter((line) => { | |
| const targetPath = "packages/types/npm/package"; | |
| if (!fs.existsSync(path.join(rootDir, "..", targetPath.split("/")[0]))) { | |
| console.warn(` ⚠️ Path may not exist`); | |
| } | |
| return line.includes(targetPath); | |
| }) |
| try { | ||
| const status = execSync("git status --porcelain", { encoding: "utf8" }) | ||
| const relevantChanges = status.split("\n").filter((line) => line.includes("packages/sdk/npm/package")) | ||
| const relevantChanges = status.split("\n").filter((line) => line.includes("packages/types/npm/package")) |
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.
Consider extracting this path to a constant at the top of the file (similar to how npmDir is defined) to avoid hardcoding and make future maintenance easier. For example:
const GIT_FILTER_PATH = "packages/types/npm/package";Then use it here as .filter((line) => line.includes(GIT_FILTER_PATH))
Important
Fix path in
commitVersionChanges()inpublish-npm.cjsto correctly filter git status forpackages/types/npm/package.commitVersionChanges()inpublish-npm.cjsfrompackages/sdk/npm/packagetopackages/types/npm/packageto ensure correct git status filtering.This description was created by
for 37b93b2. You can customize this summary. It will automatically update as commits are pushed.