feat(npm): use pnpm publish for npm_package

[luciomartinez]

What

Changes npm_package(publishable = True) so the generated <name>.publish target runs pnpm publish instead of npm publish.

This removes the proposed publish_tool API and keeps the public target shape unchanged. A pnpm_binary override remains available for custom pnpm repository names; by default it uses @pnpm//:pnpm.

The publish wrapper now:

• invokes pnpm with the generated package directory from bazel-bin
• uses a temporary cwd when BUILD_WORKSPACE_DIRECTORY is available, copying in only pnpm-workspace.yaml and .npmrc when present
• adds --no-git-checks because Bazel publishes generated output
• keeps include_npm = True for pnpm <=10, where registry operations may delegate to npm

Why

pnpm publish handles pnpm workspace publishing semantics such as replacing catalog: dependency specs before packing. npm publish does not understand those pnpm-specific specs.

The isolated temporary cwd exposes the workspace-level pnpm settings required for publishing without making the source workspace the child process cwd. Using pnpm directly also avoids adding a new publish-tool selection API while preserving the existing <name>.publish entrypoint for users.

Notes

• The catalog regression test verifies that pnpm rewrites the published package.json, not only that dry-run publish exits successfully.
• User-level .npmrc and environment-based configuration remain available through the inherited process environment.
• pnpm 11 publishes natively; the npm fallback path is only relevant for older pnpm versions.

[CLAassistant]

All committers have signed the CLA.

[jbedard]

I wonder if we should be using pnpm publish 100% of the time? With js_binary(include_npm) I assume pnpm publish will use the correct npm if/when it delegates to npm. Do you know any reason not to use pnpm publish?

[jbedard]

Some history, originally it was decided that pnpm publish did not add enough value and there were concerns about how it will find npm when delegating to npm publish.

IMO your reasons for this PR show that using pnpm publish does add enough value today, especially with things like catalog deps, but also with pnpm 11 it seems it no longer delegates to npm at all (the note at the top of the docs).

@luciomartinez want to try just using pnpm publish 100% of the time?

[luciomartinez]

Thanks! I pushed fixes as suggested:

• npm_publish_with_tool.mjs now adds --no-git-checks itself, so callers do not need to know about the bazel-bin/git-checks detail.
• Added the include_npm note for pnpm <=10; per pnpm docs, pnpm 11 publishes natively and no longer delegates to npm.
• Removed --no-git-checks from the test invocation so the test exercises the wrapper default.

On using pnpm publish 100% of the time: I do not have a technical objection from our use case. I kept this opt-in/default-npm because existing publishable = True users do not currently need a pnpm repo and may rely on npm publish semantics, so making pnpm the only/default path feels like a broader compatibility change. Happy to switch direction if maintainers prefer pnpm-only here.

[jbedard]

@codex

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 881c8067a3

ℹ️ About Codex in GitHub

Your team has set up Codex to 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 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[luciomartinez]

CI note: two checks are currently failing:

• test (e2e/js_image_oci)
• test (e2e/js_image_oci, bazel-9-no-execroot-entry-point)

The useful error I found in the failed job logs is:

ERROR: No test targets were found, yet testing was requested

This looks unrelated to this PR: the changes here are limited to npm_package publish wrapper behavior and publish tests, while the failing jobs are e2e/js_image_oci. Other js_image_oci matrix variants passed, including bazel-8 and bazel-9, and the npm-related checks/tests are green. I would expect a rerun of the failed jobs to be the next useful step.

[jbedard]

That might be github being flaky?

ERROR: /home/runner/.bazel/external/jq.bzl+/jq/toolchain/BUILD:5:15: @@jq.bzl+//jq/toolchain:type depends on @@jq.bzl+//:package_metadata in repository @@jq.bzl+ which failed to fetch. no such package '@@package_metadata+//rules': java.io.IOException: Error downloading [https://github.com/bazel-contrib/supply-chain/releases/download/v0.0.6/supply-chain-v0.0.6.tar.gz] to /home/runner/.bazel/external/package_metadata+/temp8929006452875241854/supply-chain-v0.0.6.tar.gz: GET returned 502 Bad Gateway or Proxy Error

[luciomartinez]

That might be github being flaky?

That's right. I'll re-trigger it by pushing fixes to the issue reported by Codex...

[jbedard]

I manually got the CI jobs to re-run and they appear to be passing 👍

[jbedard]

Regarding using pnpm publish 100% of the time... a robot (2 actually) told me that the CLI args are (intentionally) almost equivalent, the differences it did point out are things that I think are not applicable in bazel or which we handled such as --no-git-checks. So I'm honestly tempted to just 100% switch and avoid the need for the publish_tool API.

If someone complains we (or I, using a robot) can maybe revert back to your publish_tool idea.

[luciomartinez]

@jbedard I'm with you buddy! I took that direction and pushed a simpler version:

• <name>.publish now always invokes pnpm publish for npm_package(publishable = True)
• removed the proposed publish_tool API
• reused the existing internal npm_publish.mjs wrapper instead of adding a second wrapper
• kept pnpm_binary as the small escape hatch for custom pnpm repo names
• kept include_npm = True with the pnpm <=10 note
• updated the publish test expectations to pnpm errors and kept the catalog rewrite assertion