Skip to content

feat(npm): use pnpm publish for npm_package#2892

Open
kirkobyte wants to merge 3 commits into
aspect-build:mainfrom
kirkobyte:kirkh/fix-buildifier-2868
Open

feat(npm): use pnpm publish for npm_package#2892
kirkobyte wants to merge 3 commits into
aspect-build:mainfrom
kirkobyte:kirkh/fix-buildifier-2868

Conversation

@kirkobyte

@kirkobyte kirkobyte commented Jun 18, 2026

Copy link
Copy Markdown

Summary

  • Switches npm_package publishing from npm publish to pnpm publish, enabling pnpm workspace features like catalog: dependency resolution
  • Adds a pnpm_binary attribute to npm_package for configuring which pnpm binary to use
  • Publishes via a temp directory that copies pnpm-workspace.yaml and .npmrc from the workspace root
  • Fixes buildifier formatting (@pnpm//:pnpm@pnpm)

This is all @luciomartinez's form #2868, just with some linting fixes :)

Test plan

  • Existing npm_package_publish tests updated to verify pnpm error messages
  • New pkg_pnpm test case validates catalog dependency resolution via pnpm publish --dry-run
  • Buildifier check passes

🤖 Generated with Claude Code

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b3cf22bc6c

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

}

const spawn = spawnSync(
path.resolve(toolPath),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Resolve external pnpm rootpaths before spawning

When the generated .publish target runs from Bazel's output tree or without runfiles, toolPath is the $(rootpath @pnpm//:pnpm) value from npm_package.bzl, and external rootpaths have the ../repo/... shape. The existing js_binary launcher has special handling to map those paths to $BAZEL_BINDIR/external/..., but path.resolve(toolPath) treats them as ordinary filesystem paths and points outside bazel-out/.../bin, so the default external @pnpm binary cannot be spawned in that mode. Use an execroot/runfiles-aware path for the pnpm tool rather than resolving the rootpath directly.

Useful? React with 👍 / 👎.

Comment on lines +57 to +60
rmSync(cleanupCwd, {
force: true,
recursive: true,
})

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Preserve pnpm's publish summary before cleanup

When callers pass pnpm's --report-summary option, pnpm publish --help says it writes pnpm-publish-summary.json in the command cwd. Under bazel run this wrapper changes pnpm's cwd to cleanupCwd, so this cleanup removes the only copy of the summary before release automation can read it. Copy the summary back to a durable location or avoid using the temporary directory for outputs.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants