Skip to content

Commit

Permalink
feat(core): prompt users to connect to nx cloud when upgrading major …
Browse files Browse the repository at this point in the history 
…versions (#9849)
  • Loading branch information
@AgentEnder
AgentEnder authored Apr 14, 2022
1 parent d1b471c commit a162bfb
Show file tree
Hide file tree
Showing 2 changed files with 41 additions and 6 deletions.
10 changes: 7 additions & 3 deletions packages/nx/src/command-line/connect-to-nx-cloud.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,9 @@ export async function connectToNxCloudUsingScan(scan: boolean): Promise<void> {
}
}

export async function connectToNxCloudCommand(): Promise<void> {
export async function connectToNxCloudCommand(
promptOverride?: string
): Promise<void> {
const nxJson = readNxJson();
const nxCloudUsed = Object.values(nxJson.tasksRunnerOptions).find(
(r) => r.runner == '@nrwl/nx-cloud'
Expand All @@ -50,14 +52,16 @@ export async function connectToNxCloudCommand(): Promise<void> {
});
}

async function connectToNxCloudPrompt() {
async function connectToNxCloudPrompt(prompt?: string) {
return await (
await import('enquirer')
)
.prompt([
{
name: 'NxCloud',
message: `Connect to Nx Cloud? (It's free and doesn't require registration.)`,
message:
prompt ??
`Connect to Nx Cloud? (It's free and doesn't require registration.)`,
type: 'select',
choices: [
{
Expand Down
37 changes: 34 additions & 3 deletions packages/nx/src/command-line/migrate.ts
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { exec, execSync } from 'child_process';
import { remove } from 'fs-extra';
import { dirname, join } from 'path';
import { gt, lt, lte } from 'semver';
import { gt, lt, lte, major, valid } from 'semver';
import { promisify } from 'util';
import {
MigrationsJson,
Expand Down Expand Up @@ -29,6 +29,7 @@ import {
resolvePackageVersionUsingRegistry,
} from '../utils/package-manager';
import { handleErrors } from '../utils/params';
import { connectToNxCloudCommand } from './connect-to-nx-cloud';

export interface ResolvedMigrationConfiguration extends MigrationsJson {
packageGroup?: NxMigrationsConfiguration['packageGroup'];
Expand Down Expand Up @@ -793,6 +794,19 @@ function updatePackageJson(
});
}

async function isMigratingToNewMajor(from: string, to: string) {
from = normalizeVersion(from);
to = ['latest', 'next'].includes(to) ? to : normalizeVersion(to);
if (!valid(from)) {
from = await resolvePackageVersionUsingRegistry('nx', from);
}
if (!valid(to)) {
to = await resolvePackageVersionUsingRegistry('nx', to);
}
console.log({ from, to });
return major(from) < major(to);
}

async function generateMigrationsJsonAndUpdatePackageJson(
root: string,
opts: {
Expand All @@ -804,11 +818,28 @@ async function generateMigrationsJsonAndUpdatePackageJson(
) {
const pmc = getPackageManagerCommand();
try {
let originalPackageJson = readJsonFile<PackageJson>(
join(root, 'package.json')
);
if (
['nx', '@nrwl/workspace'].includes(opts.targetPackage) &&
(await isMigratingToNewMajor(
originalPackageJson.devDependencies?.['nx'] ??
originalPackageJson.devDependencies?.['@nrwl/workspace'],
opts.targetVersion
))
) {
await connectToNxCloudCommand(
'We noticed you are migrating to a new major version, but are not taking advantage of Nx Cloud. Nx Cloud can make your CI up to 10 times faster. Learn more about it here: nx.app. Would you like to add it?'
);
originalPackageJson = readJsonFile<PackageJson>(
join(root, 'package.json')
);
}

This comment has been minimized.

Sign in to view

Copy link
@fbartho

fbartho May 9, 2022

Re: #10144

Minimal fix:

    if (
       ['nx', '@nrwl/workspace'].includes(opts.targetPackage) &&
       (await isMigratingToNewMajor(
         originalPackageJson.devDependencies?.['nx'] ??
           originalPackageJson.devDependencies?.['@nrwl/workspace'] ⁇
           originalPackageJson.dependencies?.['nx'] ??
           originalPackageJson.dependencies?.['@nrwl/workspace'],
         opts.targetVersion
       ))
     ) {
       await connectToNxCloudCommand(
         'We noticed you are migrating to a new major version, but are not taking advantage of Nx Cloud. Nx Cloud can make your CI up to 10 times faster. Learn more about it here: nx.app. Would you like to add it?'
       );
       originalPackageJson = readJsonFile<PackageJson>(
         join(root, 'package.json')
       );
     }

Slightly more robust fix:

    try {
        if (
            ['nx', '@nrwl/workspace'].includes(opts.targetPackage) &&
            (await isMigratingToNewMajor(
                originalPackageJson.devDependencies?.['nx'] ??
                originalPackageJson.devDependencies?.['@nrwl/workspace'] ⁇
                originalPackageJson.dependencies?.['nx'] ??
                originalPackageJson.dependencies?.['@nrwl/workspace'],
                opts.targetVersion
            ))
        ) {
            await connectToNxCloudCommand(
                'We noticed you are migrating to a new major version, but are not taking advantage of Nx Cloud. Nx Cloud can make your CI up to 10 times faster. Learn more about it here: nx.app. Would you like to add it?'
            );
            originalPackageJson = readJsonFile<PackageJson>(
                join(root, 'package.json')
            );
        }
    } catch (e) {
        console.error("Failed to promote Nx Cloud:", e);
        console.log("Continuing without Nx Cloud…");
    }

Additionally, it would be drastically more helpful if normalizeVersion threw a useful error on undefined inputs

This comment has been minimized.

Sign in to view

Copy link
@fbartho

fbartho May 9, 2022 •
edited
Loading

Heya @AgentEnder this code is unsafe, and is the root cause for #10144

Specifically, it assumes that devDependencies exists and has versions for either package. Then it passes undefined to isMigratingToNewMajor which then passes undefined to normalizeVersion which expects a non-null string, so then that throws an error.

Further context on why: We use @manykg/cli to manage our monorepo dependencies, which has an additional rule that the top level package.json in a monorepo doesn't have a distinction between shipping deps and devDeps, so it recommends and enforces that you should just have one dependencies hash.

The Migrator code below takes this into account, but this marketing code does not. Using stricter null-type checking on your tsconfig or linter settings would have flagged this section as potentially problematic.
logger.info(`Fetching meta data about packages.`);
logger.info(`It may take a few minutes.`);

const originalPackageJson = readJsonFile(join(root, 'package.json'));

const migrator = new Migrator({
packageJson: originalPackageJson,
versions: versions(root, opts.from),
Expand Down

0 comments on commit a162bfb

Please sign in to comment.