Skip to content

fix(release): do not require the npm CLI to be installed for publishing JS packages with bun#32252

Closed
binsky08 wants to merge 6 commits into
nrwl:masterfrom
binsky08:fix/32126
Closed

fix(release): do not require the npm CLI to be installed for publishing JS packages with bun#32252
binsky08 wants to merge 6 commits into
nrwl:masterfrom
binsky08:fix/32126

Conversation

@binsky08

@binsky08 binsky08 commented Aug 7, 2025

Copy link
Copy Markdown

Current Behavior

nx release publish requires the npm package manager to run the hard-coded npm view ... command even in a project that doesn't use npm as package manager

Expected Behavior

nx release publish uses the detected package manager of the project (e.g. bun) to run a package manager specific command like npm view ... / bun info ...

Related Issue(s)

Fixes #32126

Notes

While nx affected --target=e2e ("No tasks were run") and nx affected --target=test ("Successfully ran target test for 43 projects and 2 tasks they depend on") were successful, I was not able to publish a local test version to a local registry - so unfortunately I couldn't manually verify whether this actually fixes the linked issue.

@binsky08 binsky08 requested review from a team as code owners August 7, 2025 14:56
@binsky08 binsky08 requested review from MaxKless and jaysoo August 7, 2025 14:56
@vercel

vercel Bot commented Aug 7, 2025

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Updated (UTC)
nx-dev Ready Preview Aug 15, 2025 2:31pm

Comment thread packages/js/src/executors/release-publish/release-publish.impl.ts

@JamesHenry JamesHenry left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks! Few comments

Comment thread packages/js/src/executors/release-publish/release-publish.impl.ts Outdated
Comment thread packages/js/src/executors/release-publish/release-publish.impl.ts Outdated
Comment thread packages/nx/src/utils/package-manager.ts Outdated
@binsky08

Copy link
Copy Markdown
Author

Thanks! Few comments

I've added a fixup with the requested changes

@nx-cloud

nx-cloud Bot commented Aug 14, 2025

Copy link
Copy Markdown
Contributor

View your CI Pipeline Execution ↗ for commit dc2caf3

Command Status Duration Result
nx affected --targets=lint,test,build,e2e,e2e-c... ❌ Failed 54m 17s View ↗
nx run-many -t check-imports check-lock-files c... ✅ Succeeded 3m 23s View ↗
nx-cloud record -- nx-cloud conformance:check ✅ Succeeded 8s View ↗
nx-cloud record -- nx format:check ✅ Succeeded 2s View ↗
nx-cloud record -- nx sync:check ✅ Succeeded <1s View ↗

☁️ Nx Cloud last updated this comment at 2026-02-27 01:24:16 UTC

@github-actions

Copy link
Copy Markdown
Contributor

Failed to publish a PR release of this pull request, triggered by @JamesHenry.
See the failed workflow run at: https://github.com/nrwl/nx/actions/runs/16963366013

@JamesHenry

Copy link
Copy Markdown
Contributor

Thanks @binsky08 I've triggered a PR release for this, it's a real npm release that we can test out in your environment and make sure it's fully working before merging. There will be an automated comment on here when it's ready to try, please let me know once you've verified that release in your bun only environment

@JamesHenry JamesHenry self-assigned this Aug 14, 2025
@JamesHenry

Copy link
Copy Markdown
Contributor

Ah looks like that is not possible due to your fork not being up to date with latest nx master (it's a precaution we have in the release script). I think we can go ahead and merge when it's green, it LGTM

@JamesHenry JamesHenry changed the title fix(release): weaken the npm requirement (npm view) when publishing packages fix(release): do not require the npm CLI to be installed for publishing JS packages with bun Aug 14, 2025

@JamesHenry JamesHenry left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@binsky08 You have some legit test failures on here.

Our AI fixer has correctly identified one of them (obviously you can use a more modern value for the npm version but it doesn't really matter):

image

auto-merge was automatically disabled August 15, 2025 14:16

Head branch was pushed to by a user without write access

@binsky08

Copy link
Copy Markdown
Author

@binsky08 You have some legit test failures on here.

Our AI fixer has correctly identified one of them (obviously you can use a more modern value for the npm version but it doesn't really matter):

image

thanks, should be fixed with 52737d3

@JamesHenry

JamesHenry commented Aug 16, 2025

Copy link
Copy Markdown
Contributor

@binsky08 Nx Cloud AI nailed the final failure too I think 😁 can you apply it via the link above?

(This)

#32252 (comment)

image

@binsky08

binsky08 commented Sep 8, 2025

Copy link
Copy Markdown
Author

@binsky08 Nx Cloud AI nailed the final failure too I think 😁 can you apply it via the link above?

(This)

#32252 (comment)

image

I'm not entirely sure about the error pattern, partly because I don't fully understand the specific test (flow). Bun says something like:

404 Not Found: http://localhost:4873/@proj%2f{project-name}
    + - “@proj/{project-name}@latest” does not exist in this registry

which seems correct to me, if the package (with the packageName from the test) does not actually exist in the registry, because the command should only be executed if !options.firstRelease applies, i.e. a package with that name already exists in the registry.

This condition already existed in the previous version of the file: https://github.com/nrwl/nx/blob/master/packages/js/src/executors/release-publish/release-publish.impl.ts#L144-L147

Therefore, I am wondering:

    1. Why is there no error with pnpm?
    1. Does the requested package really not exist in the test registry?
    • If so, the error would be correct and we may miss the firstRelease option in the test?

Unfortunately I can't reproduct the error (needed to validate my second point ^) since it (earlier) fails to run the e2e test in the proposed devcontainer.

image

Seems to be an issue with connecting to the automatically started local registry. I tried it several times and have already waited >10 minutes. Does not work.
I am not deep enough in nx to fix it when I can't test it.

I'm not sure how to proceed here.

jaysoo

This comment was marked as outdated.

@netlify

netlify Bot commented Feb 26, 2026

Copy link
Copy Markdown

Deploy Preview for nx-docs ready!

Name Link
🔨 Latest commit dc2caf3
🔍 Latest deploy log https://app.netlify.com/projects/nx-docs/deploys/69a0e47ce2e7960008cc4112
😎 Deploy Preview https://deploy-preview-32252--nx-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@netlify

netlify Bot commented Feb 26, 2026

Copy link
Copy Markdown

Deploy Preview for nx-dev ready!

Name Link
🔨 Latest commit dc2caf3
🔍 Latest deploy log https://app.netlify.com/projects/nx-dev/deploys/69a0e47c18c1bd0008c4cf3f
😎 Deploy Preview https://deploy-preview-32252--nx-dev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

nx-cloud[bot]

This comment was marked as outdated.

@jaysoo jaysoo requested a review from Coly010 as a code owner February 27, 2026 00:25

@nx-cloud nx-cloud Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nx Cloud is proposing a fix for your failed CI:

These changes fix the e2e test failures by properly handling Bun's package manager command syntax differences. We updated the view command construction to only add field specifiers for npm/pnpm (not Bun/Yarn), improved error handling to support Bun's plain-text error format, and filtered npm version warnings from test snapshots.

Warning

We could not verify this fix.

Suggested Fix changes
diff --git a/e2e/release/src/independent-projects.workspaces.test.ts b/e2e/release/src/independent-projects.workspaces.test.ts
index 996f5aaee2..e2f825e73d 100644
--- a/e2e/release/src/independent-projects.workspaces.test.ts
+++ b/e2e/release/src/independent-projects.workspaces.test.ts
@@ -54,6 +54,8 @@ expect.addSnapshotSerializer({
         .replaceAll(e2eRegistryUrl, '{registryUrl}')
         // Filter out plugin worker verbose logs
         .replaceAll(/\[(isolated-plugin|plugin-worker)\].*\n/g, '')
+        // Filter out npm warnings that may vary by npm version
+        .replaceAll(/npm warn .*\n/g, '')
         // We trim each line to reduce the chances of snapshot flakiness
         .split('\n')
         .map((r) => r.trim())
diff --git a/e2e/release/src/preserve-local-dependency-protocols.test.ts b/e2e/release/src/preserve-local-dependency-protocols.test.ts
index 00066f8940..12ff27e784 100644
--- a/e2e/release/src/preserve-local-dependency-protocols.test.ts
+++ b/e2e/release/src/preserve-local-dependency-protocols.test.ts
@@ -49,6 +49,8 @@ expect.addSnapshotSerializer({
         )
         // Filter out plugin worker verbose logs
         .replaceAll(/\[(isolated-plugin|plugin-worker)\].*\n/g, '')
+        // Filter out npm warnings that may vary by npm version
+        .replaceAll(/npm warn .*\n/g, '')
 
         .split('\n')
         .map((r) => r.trim())
diff --git a/packages/js/src/executors/release-publish/release-publish.impl.ts b/packages/js/src/executors/release-publish/release-publish.impl.ts
index 687a71c26c..18bfd4efef 100644
--- a/packages/js/src/executors/release-publish/release-publish.impl.ts
+++ b/packages/js/src/executors/release-publish/release-publish.impl.ts
@@ -154,8 +154,9 @@ Please update the local dependency on "${depName}" to be a valid semantic versio
     packageName,
   ];
 
-  // Since only yarn does not support multiple specific fields, we'll then fetch the complete data
-  if (pm !== 'yarn') {
+  // Only npm and pnpm support specifying multiple specific fields like 'versions dist-tags'
+  // yarn and bun return complete package info without field specification
+  if (pm === 'npm' || pm === 'pnpm') {
     npmViewCommandSegments.push('versions dist-tags');
   }
   npmViewCommandSegments.push(`--json --"${registryConfigKey}=${registry}"`);
@@ -276,25 +277,47 @@ Please update the local dependency on "${depName}" to be a valid semantic versio
         }
       }
     } catch (err) {
-      const stdoutData = JSON.parse(err.stdout?.toString() || '{}');
-      // If the error is that the package doesn't exist, then we can ignore it because we will be publishing it for the first time in the next step
-      if (
-        !(
-          stdoutData.error?.code?.includes('E404') &&
-          stdoutData.error?.summary?.toLowerCase().includes('not found')
-        ) &&
-        !(
-          err.stderr?.toString().includes('E404') &&
-          err.stderr?.toString().toLowerCase().includes('not found')
-        )
-      ) {
-        console.error(
-          `Something unexpected went wrong when checking for existing dist-tags.\n`,
-          err
-        );
-        return {
-          success: false,
-        };
+      try {
+        const stdoutData = JSON.parse(err.stdout?.toString() || '{}');
+        // If the error is that the package doesn't exist, then we can ignore it because we will be publishing it for the first time in the next step
+        if (
+          !(
+            stdoutData.error?.code?.includes('E404') &&
+            stdoutData.error?.summary?.toLowerCase().includes('not found')
+          ) &&
+          !(
+            err.stderr?.toString().includes('E404') &&
+            err.stderr?.toString().toLowerCase().includes('not found')
+          )
+        ) {
+          console.error(
+            `Something unexpected went wrong when checking for existing dist-tags.\n`,
+            err
+          );
+          return {
+            success: false,
+          };
+        }
+      } catch (parseErr) {
+        // If we can't parse JSON (e.g., bun returns plain text errors), check stderr for 404
+        const stderrStr = err.stderr?.toString() || '';
+        const stdoutStr = err.stdout?.toString() || '';
+        if (
+          !(
+            (stderrStr.includes('404') &&
+              stderrStr.toLowerCase().includes('not found')) ||
+            (stdoutStr.includes('404') &&
+              stdoutStr.toLowerCase().includes('not found'))
+          )
+        ) {
+          console.error(
+            `Something unexpected went wrong when checking for existing dist-tags.\n`,
+            err
+          );
+          return {
+            success: false,
+          };
+        }
       }
     }
   }

🔔 Heads up, your workspace has pending recommendations ↗ to auto-apply fixes for similar failures.

Because this branch comes from a fork, it is not possible for us to apply fixes directly, but you can apply the changes locally using the available options below.

Apply changes locally with:

npx nx-cloud apply-locally MpnO-OvaY

Apply fix locally with your editor ↗   View interactive diff ↗



🎓 Learn more about Self-Healing CI on nx.dev

@JamesHenry

Copy link
Copy Markdown
Contributor

Sorry for the delay on this @binsky08, I merged in #34835 to address it

@github-actions

Copy link
Copy Markdown
Contributor

This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request.

@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Mar 20, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Publishing packages fails if using bun and the npm executable is not available in the environment

3 participants