Skip to content

Commit

Permalink
fix(@schematics/angular): don't show server routing prompt when using…
Browse files Browse the repository at this point in the history
… `browser` builder

The new routing APIs don't support `browser` builder, but calling `ng add @angular/ssr` with a `browser` builder would still prompt the user to add them. If the user said "Yes", it would actually ignore that answer and not enable the new APIs.

With this change, `ng add @angular/ssr` when using `browser` builder does not show the prompt and assumes the answer is "No". It also throws an error if the user runs `ng add @angular/ssr --server-routing`.

I'm not aware of a built-in prompting mechanism in schematics beyond `x-prompt`, which can't be used here, so instead I just called Inquirer directly. Unfortunately testing the prompt is a little awkward, as Inquirier does not provide useful APIs in this space. I evaluated `@inquirer/testing`, but ultimately decided that was more intended for testing custom Inquirer prompts, not mocking usage of standard prompts. Schematics APIs do not provide a useful way to inject additional data like a mock, so instead I had to do this through a `setPrompterForTestOnly` function. I'm not a huge fan of it, but I don't see a more straightforward way of solving the problem.

(cherry picked from commit 160dee3)
  • Loading branch information
dgp1130 committed Nov 18, 2024
1 parent 2551df5 commit 4b4e000
Show file tree
Hide file tree
Showing 12 changed files with 243 additions and 21 deletions.
1 change: 1 addition & 0 deletions packages/angular/ssr/schematics/ng-add/index_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import { join } from 'node:path';
describe('@angular/ssr ng-add schematic', () => {
const defaultOptions = {
project: 'test-app',
serverRouting: false,
};

const schematicRunner = new SchematicTestRunner(
Expand Down
1 change: 1 addition & 0 deletions packages/schematics/angular/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ ts_library(
"//packages/angular_devkit/schematics",
"//packages/angular_devkit/schematics/tasks",
"//packages/schematics/angular/third_party/github.com/Microsoft/TypeScript",
"@npm//@inquirer/prompts",
"@npm//@types/node",
"@npm//browserslist",
"@npm//jsonc-parser",
Expand Down
1 change: 1 addition & 0 deletions packages/schematics/angular/application/index_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ describe('Application Schematic', () => {
const defaultOptions: ApplicationOptions = {
name: 'foo',
skipPackageJson: false,
serverRouting: false,
};

let workspaceTree: UnitTestTree;
Expand Down
63 changes: 58 additions & 5 deletions packages/schematics/angular/ssr/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ import { ProjectDefinition, getWorkspace } from '../utility/workspace';
import { Builders } from '../utility/workspace-models';

import { Schema as SSROptions } from './schema';
import { isTTY } from './tty';

const SERVE_SSR_TARGET_NAME = 'serve-ssr';
const PRERENDER_TARGET_NAME = 'prerender';
Expand Down Expand Up @@ -85,7 +86,7 @@ async function getApplicationBuilderOutputPaths(
const { outputPath } = architectTarget.options;
if (outputPath === null || outputPath === undefined) {
throw new SchematicsException(
`outputPath for ${projectName} ${target} target is undeined or null.`,
`outputPath for ${projectName} ${target} target is undefined or null.`,
);
}

Expand Down Expand Up @@ -361,19 +362,20 @@ function addServerFile(
};
}

export default function (options: SSROptions): Rule {
export default function (inputOptions: SSROptions): Rule {
return async (host, context) => {
const browserEntryPoint = await getMainFilePath(host, options.project);
const browserEntryPoint = await getMainFilePath(host, inputOptions.project);
const isStandalone = isStandaloneApp(host, browserEntryPoint);

const workspace = await getWorkspace(host);
const clientProject = workspace.projects.get(options.project);
const clientProject = workspace.projects.get(inputOptions.project);
if (!clientProject) {
throw targetBuildNotFoundError();
}

const isUsingApplicationBuilder = usingApplicationBuilder(clientProject);

const serverRouting = await isServerRoutingEnabled(isUsingApplicationBuilder, inputOptions);
const options = { ...inputOptions, serverRouting };
const sourceRoot = clientProject.sourceRoot ?? posix.join(clientProject.root, 'src');

return chain([
Expand Down Expand Up @@ -404,3 +406,54 @@ function usingApplicationBuilder(project: ProjectDefinition) {

return isUsingApplicationBuilder;
}

// Wrap inquirer in a `prompt` function.
export type Prompt = (message: string, defaultValue: boolean) => Promise<boolean>;
const defaultPrompter: Prompt = async (message, defaultValue) => {
const { confirm } = await import('@inquirer/prompts');

return await confirm({
message,
default: defaultValue,
});
};

// Allow the prompt functionality to be overridden to facilitate testing.
let prompt = defaultPrompter;
export function setPrompterForTestOnly(prompter?: Prompt): void {
prompt = prompter ?? defaultPrompter;
}

/** Returns whether or not server routing is enabled, potentially prompting the user if necessary. */
async function isServerRoutingEnabled(
isUsingApplicationBuilder: boolean,
options: SSROptions,
): Promise<boolean> {
if (!isUsingApplicationBuilder) {
if (options.serverRouting) {
throw new SchematicsException(
'Server routing APIs can only be added to a project using `application` builder.',
);
} else {
return false;
}
}

// Use explicit option if provided.
if (options.serverRouting !== undefined) {
return options.serverRouting;
}

const serverRoutingDefault = false;

// Use the default if not in an interactive terminal.
if (!isTTY()) {
return serverRoutingDefault;
}

// Prompt the user if in an interactive terminal and no option was provided.
return await prompt(
'Would you like to use the Server Routing and App Engine APIs (Developer Preview) for this server application?',
/* defaultValue */ serverRoutingDefault,
);
}
102 changes: 102 additions & 0 deletions packages/schematics/angular/ssr/index_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,12 @@ import { SchematicTestRunner, UnitTestTree } from '@angular-devkit/schematics/te

import { join } from 'node:path';
import { Schema as ServerOptions } from './schema';
import { Prompt, setPrompterForTestOnly } from './index';

describe('SSR Schematic', () => {
const defaultOptions: ServerOptions = {
project: 'test-app',
serverRouting: false,
};

const schematicRunner = new SchematicTestRunner(
Expand All @@ -30,6 +32,10 @@ describe('SSR Schematic', () => {
};

beforeEach(async () => {
setPrompterForTestOnly((message) => {
return fail(`Unmocked prompt: ${message}`) as never;
});

appTree = await schematicRunner.runExternalSchematic(
'@schematics/angular',
'workspace',
Expand Down Expand Up @@ -81,6 +87,8 @@ describe('SSR Schematic', () => {
});

describe('standalone application', () => {
const originalTty = process.env['NG_FORCE_TTY'];

beforeEach(async () => {
appTree = await schematicRunner.runExternalSchematic(
'@schematics/angular',
Expand All @@ -98,6 +106,10 @@ describe('SSR Schematic', () => {
);
});

afterEach(() => {
process.env['NG_FORCE_TTY'] = originalTty;
});

it('should add script section in package.json', async () => {
const tree = await schematicRunner.runSchematic('ssr', defaultOptions, appTree);
const { scripts } = tree.readJson('/package.json') as { scripts: Record<string, string> };
Expand Down Expand Up @@ -150,6 +162,74 @@ describe('SSR Schematic', () => {
server: 'node-server',
});
});

it('generates server routing configuration when enabled', async () => {
const tree = await schematicRunner.runSchematic(
'ssr',
{ ...defaultOptions, serverRouting: true },
appTree,
);

expect(tree.exists('/projects/test-app/src/app/app.routes.server.ts')).toBeTrue();
});

it('does not generate server routing configuration when disabled', async () => {
const tree = await schematicRunner.runSchematic(
'ssr',
{ ...defaultOptions, serverRouting: false },
appTree,
);

expect(tree.exists('/projects/test-app/src/app/app.routes.server.ts')).toBeFalse();
});

it('generates server routing configuration when prompt is accepted by the user', async () => {
const prompter = jasmine.createSpy<Prompt>('prompt').and.resolveTo(true);
setPrompterForTestOnly(prompter);

process.env['NG_FORCE_TTY'] = 'TRUE';
const tree = await schematicRunner.runSchematic(
'ssr',
{ ...defaultOptions, serverRouting: undefined },
appTree,
);

expect(prompter).toHaveBeenCalledTimes(1);

expect(tree.exists('/projects/test-app/src/app/app.routes.server.ts')).toBeTrue();
});

it('does not generate server routing configuration when prompt is rejected by the user', async () => {
const prompter = jasmine.createSpy<Prompt>('prompt').and.resolveTo(false);
setPrompterForTestOnly(prompter);

process.env['NG_FORCE_TTY'] = 'TRUE';
const tree = await schematicRunner.runSchematic(
'ssr',
{ ...defaultOptions, serverRouting: undefined },
appTree,
);

expect(prompter).toHaveBeenCalledTimes(1);

expect(tree.exists('/projects/test-app/src/app/app.routes.server.ts')).toBeFalse();
});

it('defaults to skipping server route generation when not in an interactive terminal', async () => {
const prompter = jasmine.createSpy<Prompt>('prompt').and.resolveTo(false);
setPrompterForTestOnly(prompter);

process.env['NG_FORCE_TTY'] = 'FALSE';
const tree = await schematicRunner.runSchematic(
'ssr',
{ ...defaultOptions, serverRouting: undefined },
appTree,
);

expect(prompter).not.toHaveBeenCalled();

expect(tree.exists('/projects/test-app/src/app/app.routes.server.ts')).toBeFalse();
});
});

describe('Legacy browser builder', () => {
Expand Down Expand Up @@ -216,5 +296,27 @@ describe('SSR Schematic', () => {
const content = tree.readContent('/projects/test-app/src/server.ts');
expect(content).toContain(`const distFolder = join(process.cwd(), 'dist/test-app/browser');`);
});

it('throws an exception when used with `serverRouting`', async () => {
await expectAsync(
schematicRunner.runSchematic('ssr', { ...defaultOptions, serverRouting: true }, appTree),
).toBeRejectedWithError(/Server routing APIs.*`application` builder/);
});

it('automatically disables `serverRouting` and does not prompt for it', async () => {
const prompter = jasmine.createSpy<Prompt>('prompt').and.resolveTo(false);
setPrompterForTestOnly(prompter);

process.env['NG_FORCE_TTY'] = 'TRUE';
const tree = await schematicRunner.runSchematic(
'ssr',
{ ...defaultOptions, serverRouting: undefined },
appTree,
);

expect(prompter).not.toHaveBeenCalled();

expect(tree.exists('/projects/test-app/src/app/app.routes.server.ts')).toBeFalse();
});
});
});
4 changes: 1 addition & 3 deletions packages/schematics/angular/ssr/schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,7 @@
},
"serverRouting": {
"description": "Creates a server application using the Server Routing and App Engine APIs (Developer Preview).",
"x-prompt": "Would you like to use the Server Routing and App Engine APIs (Developer Preview) for this server application?",
"type": "boolean",
"default": false
"type": "boolean"
}
},
"required": ["project"],
Expand Down
22 changes: 22 additions & 0 deletions packages/schematics/angular/ssr/tty.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
/**
* @license
* Copyright Google LLC All Rights Reserved.
*
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.dev/license
*/

function _isTruthy(value: undefined | string): boolean {
// Returns true if value is a string that is anything but 0 or false.
return value !== undefined && value !== '0' && value.toUpperCase() !== 'FALSE';
}

export function isTTY(): boolean {
// If we force TTY, we always return true.
const force = process.env['NG_FORCE_TTY'];
if (force !== undefined) {
return _isTruthy(force);
}

return !!process.stdout.isTTY && !_isTruthy(process.env['CI']);
}
Original file line number Diff line number Diff line change
Expand Up @@ -32,15 +32,28 @@ export default async function () {

// Forcibly remove in case another test doesn't clean itself up.
await rimraf('node_modules/@angular/ssr');
await ng(
'add',
'@angular/ssr',
'--project',
projectName,
'--skip-confirmation',
'--skip-install',
'--server-routing',
);
if (useWebpackBuilder) {
await ng(
'add',
'@angular/ssr',
'--project',
projectName,
'--skip-confirmation',
'--skip-install',
// Server routing is not supported on `browser` builder.
// '--server-routing',
);
} else {
await ng(
'add',
'@angular/ssr',
'--project',
projectName,
'--skip-confirmation',
'--skip-install',
'--server-routing',
);
}

await useSha();
await installWorkspacePackages();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,13 @@ export default async function () {
const useWebpackBuilder = !getGlobalVariable('argv')['esbuild'];
// forcibly remove in case another test doesn't clean itself up
await rimraf('node_modules/@angular/ssr');
await ng('add', '@angular/ssr', '--server-routing', '--skip-confirmation', '--skip-install');

if (useWebpackBuilder) {
// `--server-routing` not supported in `browser` builder.
await ng('add', '@angular/ssr', '--skip-confirmation', '--skip-install');
} else {
await ng('add', '@angular/ssr', '--server-routing', '--skip-confirmation', '--skip-install');
}

await useSha();
await installWorkspacePackages();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,16 @@ import { updateJsonFile, updateServerFileForWebpack, useSha } from '../../../uti
export default async function () {
// forcibly remove in case another test doesn't clean itself up
await rimraf('node_modules/@angular/ssr');
await ng('add', '@angular/ssr', '--server-routing', '--skip-confirmation', '--skip-install');

const useWebpackBuilder = !getGlobalVariable('argv')['esbuild'];

if (useWebpackBuilder) {
// `--server-routing` not supported in `browser` builder.
await ng('add', '@angular/ssr', '--skip-confirmation', '--skip-install');
} else {
await ng('add', '@angular/ssr', '--server-routing', '--skip-confirmation', '--skip-install');
}

if (!useWebpackBuilder) {
// Disable prerendering
await updateJsonFile('angular.json', (json) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,20 @@ import { killAllProcesses, ng } from '../../../utils/process';
import { rimraf, writeMultipleFiles } from '../../../utils/fs';
import { installWorkspacePackages } from '../../../utils/packages';
import { ngServe, useSha } from '../../../utils/project';
import { getGlobalVariable } from '../../../utils/env';

export default async function () {
const useWebpackBuilder = !getGlobalVariable('argv')['esbuild'];

// Forcibly remove in case another test doesn't clean itself up.
await rimraf('node_modules/@angular/ssr');
await ng('add', '@angular/ssr', '--server-routing', '--skip-confirmation');
if (useWebpackBuilder) {
// `--server-routing` not supported in `browser` builder.
await ng('add', '@angular/ssr', '--skip-confirmation', '--skip-install');
} else {
await ng('add', '@angular/ssr', '--server-routing', '--skip-confirmation', '--skip-install');
}

await useSha();
await installWorkspacePackages();

Expand Down
Loading

0 comments on commit 4b4e000

Please sign in to comment.