From 4b4e000dd60bb43df5c8694eb8a0bc0b45d1cf8d Mon Sep 17 00:00:00 2001 From: Doug Parker Date: Fri, 15 Nov 2024 21:05:22 -0800 Subject: [PATCH] fix(@schematics/angular): don't show server routing prompt when using `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 160dee33d72cbc8efbff80a4974b64f83603a95c) --- .../ssr/schematics/ng-add/index_spec.ts | 1 + packages/schematics/angular/BUILD.bazel | 1 + .../angular/application/index_spec.ts | 1 + packages/schematics/angular/ssr/index.ts | 63 ++++++++++- packages/schematics/angular/ssr/index_spec.ts | 102 ++++++++++++++++++ packages/schematics/angular/ssr/schema.json | 4 +- packages/schematics/angular/ssr/tty.ts | 22 ++++ .../prerender/discover-routes-ngmodule.ts | 31 ++++-- .../express-engine-csp-nonce.ts | 8 +- .../express-engine-standalone.ts | 9 +- .../serve/ssr-http-requests-assets.ts | 11 +- .../e2e/tests/vite/ssr-error-stack.ts | 11 +- 12 files changed, 243 insertions(+), 21 deletions(-) create mode 100644 packages/schematics/angular/ssr/tty.ts diff --git a/packages/angular/ssr/schematics/ng-add/index_spec.ts b/packages/angular/ssr/schematics/ng-add/index_spec.ts index b93a509200b1..bdf5474e0d70 100644 --- a/packages/angular/ssr/schematics/ng-add/index_spec.ts +++ b/packages/angular/ssr/schematics/ng-add/index_spec.ts @@ -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( diff --git a/packages/schematics/angular/BUILD.bazel b/packages/schematics/angular/BUILD.bazel index f75ebae5bbda..1cfba5801f09 100644 --- a/packages/schematics/angular/BUILD.bazel +++ b/packages/schematics/angular/BUILD.bazel @@ -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", diff --git a/packages/schematics/angular/application/index_spec.ts b/packages/schematics/angular/application/index_spec.ts index bac40b684402..cf0380aeddac 100644 --- a/packages/schematics/angular/application/index_spec.ts +++ b/packages/schematics/angular/application/index_spec.ts @@ -32,6 +32,7 @@ describe('Application Schematic', () => { const defaultOptions: ApplicationOptions = { name: 'foo', skipPackageJson: false, + serverRouting: false, }; let workspaceTree: UnitTestTree; diff --git a/packages/schematics/angular/ssr/index.ts b/packages/schematics/angular/ssr/index.ts index 10dc255e8c0b..9aa44f6686d5 100644 --- a/packages/schematics/angular/ssr/index.ts +++ b/packages/schematics/angular/ssr/index.ts @@ -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'; @@ -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.`, ); } @@ -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([ @@ -404,3 +406,54 @@ function usingApplicationBuilder(project: ProjectDefinition) { return isUsingApplicationBuilder; } + +// Wrap inquirer in a `prompt` function. +export type Prompt = (message: string, defaultValue: boolean) => Promise; +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 { + 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, + ); +} diff --git a/packages/schematics/angular/ssr/index_spec.ts b/packages/schematics/angular/ssr/index_spec.ts index 63c11e772c2f..7f917f8d4df6 100644 --- a/packages/schematics/angular/ssr/index_spec.ts +++ b/packages/schematics/angular/ssr/index_spec.ts @@ -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( @@ -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', @@ -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', @@ -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 }; @@ -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').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').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').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', () => { @@ -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').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(); + }); }); }); diff --git a/packages/schematics/angular/ssr/schema.json b/packages/schematics/angular/ssr/schema.json index aead824b9be6..788ad7fec3d4 100644 --- a/packages/schematics/angular/ssr/schema.json +++ b/packages/schematics/angular/ssr/schema.json @@ -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"], diff --git a/packages/schematics/angular/ssr/tty.ts b/packages/schematics/angular/ssr/tty.ts new file mode 100644 index 000000000000..0d669c0301e3 --- /dev/null +++ b/packages/schematics/angular/ssr/tty.ts @@ -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']); +} diff --git a/tests/legacy-cli/e2e/tests/build/prerender/discover-routes-ngmodule.ts b/tests/legacy-cli/e2e/tests/build/prerender/discover-routes-ngmodule.ts index 4775fa27c306..69a84a6248be 100644 --- a/tests/legacy-cli/e2e/tests/build/prerender/discover-routes-ngmodule.ts +++ b/tests/legacy-cli/e2e/tests/build/prerender/discover-routes-ngmodule.ts @@ -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(); diff --git a/tests/legacy-cli/e2e/tests/build/server-rendering/express-engine-csp-nonce.ts b/tests/legacy-cli/e2e/tests/build/server-rendering/express-engine-csp-nonce.ts index f7d98bdd3dbc..e7bd9d9b1ecb 100644 --- a/tests/legacy-cli/e2e/tests/build/server-rendering/express-engine-csp-nonce.ts +++ b/tests/legacy-cli/e2e/tests/build/server-rendering/express-engine-csp-nonce.ts @@ -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(); diff --git a/tests/legacy-cli/e2e/tests/build/server-rendering/express-engine-standalone.ts b/tests/legacy-cli/e2e/tests/build/server-rendering/express-engine-standalone.ts index cfc3ea84a196..632c90522a3e 100644 --- a/tests/legacy-cli/e2e/tests/build/server-rendering/express-engine-standalone.ts +++ b/tests/legacy-cli/e2e/tests/build/server-rendering/express-engine-standalone.ts @@ -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) => { diff --git a/tests/legacy-cli/e2e/tests/commands/serve/ssr-http-requests-assets.ts b/tests/legacy-cli/e2e/tests/commands/serve/ssr-http-requests-assets.ts index ecc6c6c1015a..972c35be4452 100644 --- a/tests/legacy-cli/e2e/tests/commands/serve/ssr-http-requests-assets.ts +++ b/tests/legacy-cli/e2e/tests/commands/serve/ssr-http-requests-assets.ts @@ -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(); diff --git a/tests/legacy-cli/e2e/tests/vite/ssr-error-stack.ts b/tests/legacy-cli/e2e/tests/vite/ssr-error-stack.ts index 0f6e5b1cb6be..7061e881fdff 100644 --- a/tests/legacy-cli/e2e/tests/vite/ssr-error-stack.ts +++ b/tests/legacy-cli/e2e/tests/vite/ssr-error-stack.ts @@ -3,11 +3,20 @@ import { ng } from '../../utils/process'; import { appendToFile, rimraf } from '../../utils/fs'; import { ngServe, useSha } from '../../utils/project'; import { installWorkspacePackages } from '../../utils/packages'; +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();