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
17 changes: 5 additions & 12 deletions e2e/release/src/private-js-packages.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -188,28 +188,21 @@ describe('nx release - private JS packages', () => {

`);

const privatePkgPublishOutput = runCLI(`release publish -p ${privatePkg}`);
const privatePkgPublishOutput = runCLI(`release publish -p ${privatePkg}`, {
silenceError: true,
});
expect(privatePkgPublishOutput).toMatchInlineSnapshot(`

> NX Your filter "{private-project-name}" matched the following projects:

- {private-project-name}


> NX Running target nx-release-publish for project {private-project-name}:
> NX Based on your config, the following projects were matched for publishing but do not have the "nx-release-publish" target specified:

- {private-project-name}



> nx run {private-project-name}:nx-release-publish

Skipped package "@proj/{private-project-name}" from project "{private-project-name}", because it has \`"private": true\` in {private-project-name}/package.json



> NX Successfully ran target nx-release-publish for project {private-project-name}

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


`);
Expand Down
12 changes: 4 additions & 8 deletions packages/nx/src/command-line/release/config/config.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1990,8 +1990,7 @@ describe('createNxReleaseConfig()', () => {
projects: '*', // using string form to ensure that is supported in addition to array form
},
},
},
'nx-release-publish'
}
);
expect(res).toMatchInlineSnapshot(`
{
Expand Down Expand Up @@ -2023,8 +2022,7 @@ describe('createNxReleaseConfig()', () => {
},
},
},
{},
'nx-release-publish'
{}
);
expect(res2).toMatchInlineSnapshot(`
{
Expand Down Expand Up @@ -2194,8 +2192,7 @@ describe('createNxReleaseConfig()', () => {
projects: '*', // using string form to ensure that is supported in addition to array form
},
},
},
'nx-release-publish'
}
);
expect(res).toMatchInlineSnapshot(`
{
Expand Down Expand Up @@ -2227,8 +2224,7 @@ describe('createNxReleaseConfig()', () => {
},
},
},
{},
'nx-release-publish'
{}
);
expect(res2).toMatchInlineSnapshot(`
{
Expand Down
50 changes: 1 addition & 49 deletions packages/nx/src/command-line/release/config/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
import { NxJsonConfiguration } from '../../../config/nx-json';
import { output, type ProjectGraph } from '../../../devkit-exports';
import { findMatchingProjects } from '../../../utils/find-matching-projects';
import { projectHasTarget } from '../../../utils/project-graph-utils';
import { resolveNxJsonConfigErrorMessage } from '../utils/resolve-nx-json-error-message';

type DeepRequired<T> = Required<{
Expand Down Expand Up @@ -73,7 +72,6 @@ export interface CreateNxReleaseConfigError {
| 'RELEASE_GROUP_MATCHES_NO_PROJECTS'
| 'RELEASE_GROUP_RELEASE_TAG_PATTERN_VERSION_PLACEHOLDER_MISSING_OR_EXCESSIVE'
| 'PROJECT_MATCHES_MULTIPLE_GROUPS'
| 'PROJECTS_MISSING_TARGET'
| 'CONVENTIONAL_COMMITS_SHORTHAND_MIXED_WITH_OVERLAPPING_GENERATOR_OPTIONS'
| 'GLOBAL_GIT_CONFIG_MIXED_WITH_GRANULAR_GIT_CONFIG';
data: Record<string, string | string[]>;
Expand All @@ -82,9 +80,7 @@ export interface CreateNxReleaseConfigError {
// Apply default configuration to any optional user configuration and handle known errors
export async function createNxReleaseConfig(
projectGraph: ProjectGraph,
userConfig: NxJsonConfiguration['release'] = {},
// Optionally ensure that all configured projects have implemented a certain target
requiredTargetName?: 'nx-release-publish'
userConfig: NxJsonConfiguration['release'] = {}
): Promise<{
error: null | CreateNxReleaseConfigError;
nxReleaseConfig: NxReleaseConfig | null;
Expand Down Expand Up @@ -353,21 +349,6 @@ export async function createNxReleaseConfig(
};
}

// Ensure all matching projects have the relevant target available, if applicable
if (requiredTargetName) {
const error = ensureProjectsHaveTarget(
matchingProjects,
projectGraph,
requiredTargetName
);
if (error) {
return {
error,
nxReleaseConfig: null,
};
}
}

// If provided, ensure release tag pattern is valid
if (releaseGroup.releaseTagPattern) {
const error = ensureReleaseGroupReleaseTagPatternIsValid(
Expand Down Expand Up @@ -526,14 +507,6 @@ export async function handleNxReleaseConfigError(
});
}
break;
case 'PROJECTS_MISSING_TARGET':
{
output.error({
title: `Based on your config, the following projects were matched for release but do not have a "${error.data.targetName}" target specified. Please ensure you have an appropriate plugin such as @nx/js installed, or have configured the target manually, or exclude the projects using release groups config in nx.json:`,
bodyLines: Array.from(error.data.projects).map((name) => `- ${name}`),
});
}
break;
case 'RELEASE_GROUP_RELEASE_TAG_PATTERN_VERSION_PLACEHOLDER_MISSING_OR_EXCESSIVE':
{
const nxJsonMessage = await resolveNxJsonConfigErrorMessage([
Expand Down Expand Up @@ -610,27 +583,6 @@ function ensureArray(value: string | string[]): string[] {
return Array.isArray(value) ? value : [value];
}

function ensureProjectsHaveTarget(
projects: string[],
projectGraph: ProjectGraph,
requiredTargetName: string
): null | CreateNxReleaseConfigError {
const missingTargetProjects = projects.filter(
(project) =>
!projectHasTarget(projectGraph.nodes[project], requiredTargetName)
);
if (missingTargetProjects.length) {
return {
code: 'PROJECTS_MISSING_TARGET',
data: {
targetName: requiredTargetName,
projects: missingTargetProjects,
},
};
}
return null;
}

function isObject(value: any): value is Record<string, any> {
return value && typeof value === 'object' && !Array.isArray(value);
}
Expand Down
24 changes: 22 additions & 2 deletions packages/nx/src/command-line/release/publish.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
readGraphFileFromGraphArg,
} from '../../utils/command-line-utils';
import { logger } from '../../utils/logger';
import { projectHasTarget } from '../../utils/project-graph-utils';
import { generateGraph } from '../graph/graph';
import { PublishOptions } from './command-object';
import {
Expand Down Expand Up @@ -47,8 +48,7 @@ export async function releasePublish(args: PublishOptions): Promise<void> {
// Apply default configuration to any optional user configuration
const { error: configError, nxReleaseConfig } = await createNxReleaseConfig(
projectGraph,
nxJson.release,
'nx-release-publish'
nxJson.release
);
if (configError) {
return await handleNxReleaseConfigError(configError);
Expand Down Expand Up @@ -172,6 +172,7 @@ async function runPublishOnProjects(
projectNames
);
} else {
ensureAllProjectsHaveTarget(projectsToRun);
/**
* Run the relevant nx-release-publish executor on each of the selected projects.
*/
Expand All @@ -198,3 +199,22 @@ async function runPublishOnProjects(
}
}
}

function ensureAllProjectsHaveTarget(projectsToRun: ProjectGraphProjectNode[]) {
const requiredTargetName = 'nx-release-publish';
const projectsMissingTarget = projectsToRun.filter(
(project) => !projectHasTarget(project, requiredTargetName)
);
if (projectsMissingTarget.length === 0) {
return;
}
output.error({
title: `Based on your config, the following projects were matched for publishing but do not have the "${requiredTargetName}" target specified:`,
bodyLines: [
...projectsMissingTarget.map((p) => `- ${p.name}`),
'',
'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

}
3 changes: 1 addition & 2 deletions packages/nx/src/command-line/release/release.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,7 @@ export async function release(
// Apply default configuration to any optional user configuration
const { error: configError, nxReleaseConfig } = await createNxReleaseConfig(
projectGraph,
nxJson.release,
'nx-release-publish'
nxJson.release
);
if (configError) {
return await handleNxReleaseConfigError(configError);
Expand Down
59 changes: 43 additions & 16 deletions packages/nx/src/command-line/release/version.ts
Original file line number Diff line number Diff line change
Expand Up @@ -107,8 +107,7 @@ export async function releaseVersion(
// Apply default configuration to any optional user configuration
const { error: configError, nxReleaseConfig } = await createNxReleaseConfig(
projectGraph,
nxJson.release,
'nx-release-publish'
nxJson.release
);
if (configError) {
return await handleNxReleaseConfigError(configError);
Expand Down Expand Up @@ -575,20 +574,48 @@ function resolveGeneratorData({
configGeneratorOptions,
projects,
}): GeneratorData {
const { normalizedGeneratorName, schema, implementationFactory } =
getGeneratorInformation(
try {
const { normalizedGeneratorName, schema, implementationFactory } =
getGeneratorInformation(
collectionName,
generatorName,
workspaceRoot,
projects
);

return {
collectionName,
generatorName,
workspaceRoot,
projects
);

return {
collectionName,
generatorName,
configGeneratorOptions,
normalizedGeneratorName,
schema,
implementationFactory,
};
configGeneratorOptions,
normalizedGeneratorName,
schema,
implementationFactory,
};
} catch (err) {
if (err.message.startsWith('Unable to resolve')) {
// See if it is because the plugin is not installed
try {
require.resolve(collectionName);
// is installed
throw new Error(
`Unable to resolve the generator called "${generatorName}" within the "${collectionName}" package`
);
} catch {
/**
* Special messaging for the most common case (especially as the user is unlikely to explicitly have
* the @nx/js generator config in their nx.json so we need to be clear about what the problem is)
*/
if (collectionName === '@nx/js') {
throw new Error(
'The @nx/js plugin is required in order to version your JavaScript packages. Please install it and try again.'
);
}
throw new Error(
`Unable to resolve the package ${collectionName} in order to load the generator called ${generatorName}. Is the package installed?`
);
}
}
// Unexpected error, rethrow
throw err;
}
}
14 changes: 0 additions & 14 deletions packages/nx/src/config/workspaces.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,20 +18,6 @@ const libConfig = (root, name?: string) => ({
},
});

const packageLibConfig = (root, name?: string) => ({
name: name ?? toProjectName(`${root}/some-file`),
root,
sourceRoot: root,
projectType: 'library',
targets: {
'nx-release-publish': {
dependsOn: ['^nx-release-publish'],
executor: '@nx/js:release-publish',
options: {},
},
},
});

describe('Workspaces', () => {
let fs: TempFs;
beforeEach(() => {
Expand Down
11 changes: 8 additions & 3 deletions packages/nx/src/utils/package-json.ts
Original file line number Diff line number Diff line change
Expand Up @@ -134,16 +134,21 @@ export function buildTargetFromScript(
};
}

export function readTargetsFromPackageJson({ scripts, nx }: PackageJson) {
export function readTargetsFromPackageJson(packageJson: PackageJson) {
const { scripts, nx } = packageJson;
const res: Record<string, TargetConfiguration> = {};
Object.keys(scripts || {}).forEach((script) => {
if (!nx?.includedScripts || nx?.includedScripts.includes(script)) {
res[script] = buildTargetFromScript(script, nx);
}
});

// Add implicit nx-release-publish target for all package.json files to allow for lightweight configuration for package based repos
if (!res['nx-release-publish']) {
/**
* Add implicit nx-release-publish target for all package.json files that are
* not marked as `"private": true` to allow for lightweight configuration for
* package based repos.
*/
if (!packageJson.private && !res['nx-release-publish']) {
JamesHenry marked this conversation as resolved.
Show resolved Hide resolved
res['nx-release-publish'] = {
dependsOn: ['^nx-release-publish'],
executor: '@nx/js:release-publish',
Expand Down
33 changes: 1 addition & 32 deletions scripts/nx-release.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
#!/usr/bin/env node
import { createProjectGraphAsync, workspaceRoot } from '@nx/devkit';
import { execSync } from 'node:child_process';
import { rmSync, writeFileSync } from 'node:fs';
import { rmSync } from 'node:fs';
import { join } from 'node:path';
import { URL } from 'node:url';
import { isRelativeVersionKeyword } from 'nx/src/command-line/release/utils/semver';
Expand Down Expand Up @@ -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

if (options.local) {
console.log(
'\nPublishing locally, so setting all resolved packages to not be private'
);
const projectGraph = await createProjectGraphAsync();
for (const proj of Object.values(projectGraph.nodes)) {
if (proj.data.targets?.['nx-release-publish']) {
const packageJsonPath = join(
workspaceRoot,
proj.data.targets?.['nx-release-publish']?.options.packageRoot,
'package.json'
);
try {
const packageJson = require(packageJsonPath);
if (packageJson.private) {
console.log(
'- Publishing private package locally:',
packageJson.name
);
writeFileSync(
packageJsonPath,
JSON.stringify({ ...packageJson, private: false })
);
}
} catch {}
}
}
}

const distTag = determineDistTag(options.version);

// Run with dynamic output-style so that we have more minimal logs by default but still always see errors
Expand Down