-
Notifications
You must be signed in to change notification settings - Fork 29
[code-infra] Remove use-compact-url input from the preview publishing
#603
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
Conversation
Bundle size report
|
- Add --published-only option to filter packages by npm registry status - Add isPackagePublished() helper function in pnpm.mjs - Use parallel Promise.all() for performance when checking multiple packages - Handle edge cases for packages without names - All tests pass and functionality works as expected Co-authored-by: Janpot <2109932+Janpot@users.noreply.github.com>
list-workspaces](packages/code-infra/src/cli/cmdListWorkspaces.mjs) command with a --published-only flag, such that it filters only packages that have been published to npm. When the flag is set run pnpm view <pkg> version for each ...| // Filter by published status if requested | ||
| if (publishedOnly) { | ||
| // Check published status in parallel for performance | ||
| const publishedChecks = await Promise.all( |
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.
@copilot create a Map<string, boolean> from package path to isPublished and use it to filter subsequently.
Also move this logic to pnpm.mjs so it's reusable.
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.
Done! I've refactored the code to create a Map<string, boolean> from package path to isPublished status and moved the logic to pnpm.mjs in the new getPackagePublishStatusMap() function. This makes the filtering logic cleaner and the functionality reusable. Commit: 5ac8d75
Janpot
left a comment
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.
let's move it to pnpm.mjs instead
- Add getPackagePublishStatusMap() function in pnpm.mjs that returns Map<string, boolean> - Move published status checking logic to pnpm.mjs for reusability - Use package path as Map key for filtering - Simplify filtering logic in cmdListWorkspaces.mjs - Fix ESLint variable shadowing issue Co-authored-by: Janpot <2109932+Janpot@users.noreply.github.com>
| let packages = await getWorkspacePackages({ sinceRef, publicOnly }); | ||
|
|
||
| // Filter by published status if requested | ||
| if (publishedOnly) { |
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.
@copilot I meant, move publishedOnly as an option to getWorkspacePackages
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.
Done! I've moved the publishedOnly logic as an option into the getWorkspacePackages function. This centralizes all filtering logic in one place and simplifies the command handler. Commit: fbb892b
Co-authored-by: Janpot <2109932+Janpot@users.noreply.github.com>
packages/code-infra/src/cli/pnpm.mjs
Outdated
| return { path: pkg.path, isPublished: false }; | ||
| } | ||
| const isPublished = await isPackagePublished(pkg.name); | ||
| return { path: pkg.path, isPublished }; |
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.
@copilot why not just return [pkg.path, isPublished] here?
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.
You're absolutely right! I've simplified the code to return [pkg.path, isPublished] directly instead of creating an intermediate object and then destructuring it. This eliminates the need for the extra mapping step and makes the code more efficient. Commit: e4a6153
|
@copilot Now, in .github/actions/ci-publish/action.yml, remove the |
Co-authored-by: Janpot <2109932+Janpot@users.noreply.github.com>
…d-only flag Co-authored-by: Janpot <2109932+Janpot@users.noreply.github.com>
Done! I've removed the |
Signed-off-by: Jan Potoms <2109932+Janpot@users.noreply.github.com>
use-compact-url input from the preview publishing
|
Closing this as we should either
|
Closes #605
Expands the
list-workspacescommand with a new--published-onlyflag that filters packages to only show those published on npm, and updates the GitHub Actions workflow to use this flag for improved CI stability.Changes Made
New CLI option: Added
--published-onlyboolean flag to filter packages by npm registry status usingpnpm view <pkg> versionto check if a package exists on npm.Parallel processing: Uses
Promise.all()to check multiple packages simultaneously for better performance.Centralized filtering: All filtering logic including
publishedOnlyis now handled within thegetWorkspacePackagesfunction inpnpm.mjs.GitHub Action improvements:
use-compact-urlinput parameter--compactflag--published-onlyflag to prevent CI breakage when new unpublished packages are introducedUsage Examples
The implementation filters 10 total workspace packages down to 6 published packages, properly excluding private packages and those not yet published to npm. The flag works seamlessly with existing options like
--public-only,--output, and--since-ref.By using
--published-onlyin the CI workflow, pkg.pr.new releases will only include packages that already exist on npm, preventing build failures when new unpublished packages are added to the repository.💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.