fix(js): show lifecycle script contents in publish executor, scrub version in dry-run #23850

JamesHenry · 2024-05-21T13:07:19Z

Current Behavior

Lifecycle scripts related to publishing change the output of npm publish, which mixes JSON with non-JSON content despite the --json flag being set. Currently, we will error when attempting to parse the whole thing as JSON.

The lifecycle scripts contents themselves are not shown to the user.

Additionally, during dry-run we have no choice but to print the version that currently exists on disk, which can be confusing.

Expected Behavior

We extract and parse the JSON from the npm publish output, even when it is mixed with other output. We also show the lifecycle script outputs to the user, where applicable.

During dry-run, we replace the version in the publish output with a placeholder to avoid confusion around what would be published.

Related Issue(s)

Fixes #22925

…rsion in dry-run

vercel · 2024-05-21T13:07:22Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment

Name	Status	Preview	Updated (UTC)
nx-dev	⬜️ Ignored (Inspect)	Visit Preview	May 24, 2024 9:13am

nx-cloud · 2024-05-21T13:07:48Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 21b243d. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution

✅ Successfully ran 6 targets

Sent with 💌 from NxCloud.

packages/js/src/executors/release-publish/extract-json-data-from-string.ts

JamesHenry · 2024-05-23T10:37:18Z

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

+    const extractJsonData = extractNpmPublishJsonData(output.toString());
+    if (!extractJsonData.jsonData) {
+      console.error(
+        'The npm publish output data could not be extracted. Please report this issue on https://github.com/nrwl/nx'


This should not occur under any known circumstances, that's why I added this request for a report

FrozenPandaz

This solution is much better and more robust 👍

Some cleanup and I think it's good to go.

packages/js/src/executors/release-publish/extract-npm-publish-json-data.ts

FrozenPandaz · 2024-05-23T17:17:39Z

packages/js/src/executors/release-publish/extract-npm-publish-json-data.ts

+  return {
+    beforeJsonData: str,
+    jsonData: null,
+    afterJsonData: '',
+  };


I think we should throw an error here instead.

I don’t think I agree, because we would then need dedicated handling within the existing try catch that this code executes within to get it to ignore this specific error only, it would currently blow up and end up in the "something went wrong" ultimate fallback. That catch is already relatively involved for handling errors from npm and that would suddenly bloat its purpose to handle known errors from us as well. It’s either that or wrap it in another nested try catch which seems equally undesirable…

Ultimately, it's subjective, with zero consequences to the user experience either way (assuming you agree with me that it shouldn't end up in the "something unexpected went wrong" section), so hopefully we don't need to block the PR over it

FrozenPandaz · 2024-05-23T17:18:12Z

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

+     * Additionally, we want to capture and show the lifecycle script outputs as beforeJsonData and afterJsonData and print them accordingly below.
+     */
+    const extractJsonData = extractNpmPublishJsonData(output.toString());
+    if (!extractJsonData.jsonData) {


If we throw an error from the function, we won't need this block.

Please see comment, we would need more code overall than this block in the error case.

packages/js/src/executors/release-publish/extract-npm-publish-json-data.ts

FrozenPandaz · 2024-05-23T17:21:18Z

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


    // If npm workspaces are in use, the publish output will nest the data under the package name, so we normalize it first
-    const normalizedStdoutData = stdoutData[packageName] ?? stdoutData;
+    const normalizedStdoutData =
+      extractJsonData.jsonData[packageName] ?? extractJsonData.jsonData;


I can't easily tell. Is the nested packageName specific form handled by the function above?

Can you ensure unit tests exist for both cases please?

As part of my testing of npm to ensure that the required keys are present in all supported versions (and beyond), I (re)discovered the fundamental change that we made to this executor a little while ago.

In order to improve the npm config resolution (and in taking advice from Luke from NPM) we stopped running npm publish from the package root and began running it from the workspace root, pointing it to the package root instead.

Well it turns out the nesting behaviour that I handled on this line a long time ago (and the comment I wrote above it) are only ever applicable when running with workspaces AND when executing the command from the package root. So this nesting never occurs in our executor any more and I therefore removed it entirely. This also lets us keep the new JSON extraction logic as simple as possible.

So that this knowledge is not lost I have added a comment above the npm publish command construction: 21b243d#diff-1c2ebfe42e321fc6f4a8c99988807e45610b304f4685123906741b0260fc5472R204-R211

…rsion in dry-run (#23850)     ## Current Behavior  Lifecycle scripts related to publishing change the output of `npm publish`, which mixes JSON with non-JSON content despite the `--json` flag being set. Currently, we will error when attempting to parse the whole thing as JSON. The lifecycle scripts contents themselves are not shown to the user. Additionally, during dry-run we have no choice but to print the version that currently exists on disk, which can be confusing. ## Expected Behavior  We extract and parse the JSON from the `npm publish` output, even when it is mixed with other output. We also show the lifecycle script outputs to the user, where applicable. During dry-run, we replace the version in the publish output with a placeholder to avoid confusion around what would be published. ## Related Issue(s)  Fixes #22925 (cherry picked from commit aa2519f)

github-actions · 2024-06-07T00:10:07Z

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.

JamesHenry added 2 commits May 21, 2024 16:38

fix(js): show lifecycle script contents in publish executor, scrub ve…

eafaf5f

…rsion in dry-run

chore(repo): update e2e snapshots for dry-run publish output

6ee6421

JamesHenry mentioned this pull request May 21, 2024

fix(release): parse NPM output for JSON to prevent JSON.parse from throwing #23112

Closed

JamesHenry marked this pull request as ready for review May 21, 2024 13:20

JamesHenry requested review from a team as code owners May 21, 2024 13:20

JamesHenry requested review from Cammisuli and Coly010 May 21, 2024 13:20

JamesHenry enabled auto-merge (squash) May 21, 2024 13:22

fahslaj approved these changes May 21, 2024

View reviewed changes

FrozenPandaz requested changes May 21, 2024

View reviewed changes

packages/js/src/executors/release-publish/extract-json-data-from-string.ts Outdated Show resolved Hide resolved

JamesHenry self-assigned this May 21, 2024

chore(js): improve resilience of npm publish json data detection

120e6f1

JamesHenry commented May 23, 2024

View reviewed changes

chore(js): only print beforeJsonData or afterJsonData if relevant

cb30228

JamesHenry requested a review from FrozenPandaz May 23, 2024 13:08

FrozenPandaz requested changes May 23, 2024

View reviewed changes

JamesHenry requested a review from FrozenPandaz May 23, 2024 20:24

fix(js): remove unnecessary nested json handling

21b243d

JamesHenry force-pushed the publish-executor-fixes branch from aa1f644 to 21b243d Compare May 24, 2024 09:10

FrozenPandaz approved these changes May 27, 2024

View reviewed changes

FrozenPandaz disabled auto-merge May 27, 2024 14:19

FrozenPandaz merged commit aa2519f into master May 27, 2024
6 checks passed

FrozenPandaz deleted the publish-executor-fixes branch May 27, 2024 14:20

github-actions bot locked as resolved and limited conversation to collaborators Jun 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(js): show lifecycle script contents in publish executor, scrub version in dry-run #23850

fix(js): show lifecycle script contents in publish executor, scrub version in dry-run #23850

JamesHenry commented May 21, 2024 •

edited

vercel bot commented May 21, 2024 •

edited

nx-cloud bot commented May 21, 2024 •

edited

JamesHenry May 23, 2024

FrozenPandaz left a comment

FrozenPandaz May 23, 2024

JamesHenry May 23, 2024 •

edited

JamesHenry May 24, 2024 •

edited

FrozenPandaz May 23, 2024

JamesHenry May 23, 2024 •

edited

FrozenPandaz May 23, 2024

JamesHenry May 24, 2024

github-actions bot commented Jun 7, 2024

fix(js): show lifecycle script contents in publish executor, scrub version in dry-run #23850

fix(js): show lifecycle script contents in publish executor, scrub version in dry-run #23850

Conversation

JamesHenry commented May 21, 2024 • edited

Current Behavior

Expected Behavior

Related Issue(s)

vercel bot commented May 21, 2024 • edited

nx-cloud bot commented May 21, 2024 • edited

☁️ Nx Cloud Report

JamesHenry May 23, 2024

Choose a reason for hiding this comment

FrozenPandaz left a comment

Choose a reason for hiding this comment

FrozenPandaz May 23, 2024

Choose a reason for hiding this comment

JamesHenry May 23, 2024 • edited

Choose a reason for hiding this comment

JamesHenry May 24, 2024 • edited

Choose a reason for hiding this comment

FrozenPandaz May 23, 2024

Choose a reason for hiding this comment

JamesHenry May 23, 2024 • edited

Choose a reason for hiding this comment

FrozenPandaz May 23, 2024

Choose a reason for hiding this comment

JamesHenry May 24, 2024

Choose a reason for hiding this comment

github-actions bot commented Jun 7, 2024

JamesHenry commented May 21, 2024 •

edited

vercel bot commented May 21, 2024 •

edited

nx-cloud bot commented May 21, 2024 •

edited

JamesHenry May 23, 2024 •

edited

JamesHenry May 24, 2024 •

edited

JamesHenry May 23, 2024 •

edited