Skip to content
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

fix(release): only add nx-release-publish to public packages #21338

Merged
merged 8 commits into from
Jan 26, 2024

Conversation

JamesHenry
Copy link
Collaborator

  • Only adds the nx-release-publish target when "private": true is not set in the package.json of a JS project
  • Moves handling of missing nx-release-publish target to the publish command
  • Provides better error handling for missing plugins or generators within plugins for the version command

Copy link

vercel bot commented Jan 25, 2024

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 Jan 26, 2024 1:09pm

@JamesHenry JamesHenry enabled auto-merge (squash) January 25, 2024 18:34
Copy link
Collaborator

@FrozenPandaz FrozenPandaz left a comment

Choose a reason for hiding this comment

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

I didn't have the chance to test this locally yet but left some initial comments

'There are a few possible reasons for this: (1) The projects may be private (2) You may not have an appropriate plugin (such as `@nx/js`) installed which adds the target automatically to public projects (3) You intended to configure the target manually, or exclude those projects via config in nx.json',
],
});
process.exit(1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could this have been thrown as an error? It's quite a lengthy message but since we have a programmatic API, it's weird for their process to suddenly exit after calling the function without a chance to catch it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes fair point, it would block programmatic consumers. We don’t have handleErrors in use in this command so I’ll double check the experience tomorrow

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So the way I have decided to handle this is leverage the handleErrors function for the CLI usage like we do for the other subcommands, but additionally have an appreciation for whether or not the code is being run via the CLI.

This allows us to keep the CLI error messaging focused on the publish run (where there are no other config errors), instead of having the runCommand failure and then another error message afterwards just reiterating that publishing has failed.

E.g. what I have gone for

image

vs

no appreciation for how it's being run via the CLI

image

packages/nx/src/utils/package-json.ts Show resolved Hide resolved
@@ -116,36 +115,6 @@ const LARGE_BUFFER = 1024 * 1000000;
if (options.dryRun) {
console.warn('Not Publishing because --dryRun was passed');
} else {
// If publishing locally, force all projects to not be private first
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is removed because @nx/js: handles this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No it’s because we agreed that the only workaround for this now was to manually set the targets instead, so I went to do that and realised we don’t have any private projects. If you want me to restore it in case we ever do something like that again in the future I can

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have restored the behaviour in case we ever need it again in future, and updated the log message to more explicitly cover the fact that a target will now need to be added manually for the private packages

@JamesHenry JamesHenry merged commit c577f48 into nrwl:master Jan 26, 2024
6 checks passed
@JamesHenry JamesHenry deleted the release-version-publish-handling branch January 26, 2024 16:57
Copy link

github-actions bot commented Feb 1, 2024

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 Feb 1, 2024
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.

3 participants