fix(server): ensure consistency between CLI serve entrypoints regardi…

…ng help and strict (#9809)

This PR aims to fix inconsistencies across all the CLI entry points
around `--help` and strict.

This PR is technically breaking but I feel like most users would
consider many of these things to be bug fixes (e.g. `--help` runs the
server 😂). @Tobbe we can talk in more detail about all the changes.

Here's a non exhaustive list of all the changes

- the `api-root-path` alias was added to `@redwoodjs/api-server` to make
the options the same across `yarn rw serve` and `yarn rw-server`
- refactored `@redwoodjs/api-server` entrypoint to handle `--help` and
yargs strict mode and; we also moved parsing into the if block cause it
was running too early (during import)
- for the CLI (`yarn rw ...`) yargs strict mode wasn’t exiting with exit
code 1 when unknown arguments were passed; now it does and it prints to
stderr in the case there is an error
- for `@redwoodjs/web-server`, passing `--help` would just run the
server since we were using `yargsParser` instead of `yargs`; also added
strict mode here and like the others; kept the options the same between
the entrypoints
- updated the server-tests tests to handle all these cases
- spiked out some more tests to write to ensure we're covering all the
similarities and differences before refactoring everything

---------

Co-authored-by: Tobbe Lundberg <tobbe@tlundberg.com>

packages/api-server/dist.test.ts

@@ -37,6 +37,7 @@ describe('dist', () => {
         "apiCliOptions": {
           "apiRootPath": {
             "alias": [
+              "api-root-path",
               "rootPath",
               "root-path",
             ],
@@ -74,7 +75,7 @@ describe('dist', () => {
         "webCliOptions": {
           "apiHost": {
             "alias": "api-host",
-            "desc": "Forward requests from the apiUrl, defined in redwood.toml to this host",
+            "desc": "Forward requests from the apiUrl, defined in redwood.toml, to this host",
             "type": "string",
           },
           "port": {

packages/api-server/src/cliHandlers.ts

@@ -29,7 +29,7 @@ export const apiCliOptions = {
   port: { default: getConfig().api?.port || 8911, type: 'number', alias: 'p' },
   socket: { type: 'string' },
   apiRootPath: {
-    alias: ['rootPath', 'root-path'],
+    alias: ['api-root-path', 'rootPath', 'root-path'],
     default: '/',
     type: 'string',
     desc: 'Root path where your api functions are served',
@@ -49,7 +49,7 @@ export const webCliOptions = {
   apiHost: {
     alias: 'api-host',
     type: 'string',
-    desc: 'Forward requests from the apiUrl, defined in redwood.toml to this host',
+    desc: 'Forward requests from the apiUrl, defined in redwood.toml, to this host',
   },
 } as const
 
@@ -128,9 +128,24 @@ export const bothServerHandler = async (options: BothServerArgs) => {
 
 export const webServerHandler = async (options: WebServerArgs) => {
   const { port, socket, apiHost } = options
+  const apiUrl = getConfig().web.apiUrl
+
+  if (!apiHost && !isFullyQualifiedUrl(apiUrl)) {
+    console.error(
+      `${c.red('Error')}: If you don't provide ${c.magenta(
+        'apiHost'
+      )}, ${c.magenta(
+        'apiUrl'
+      )} needs to be a fully-qualified URL. But ${c.magenta(
+        'apiUrl'
+      )} is ${c.yellow(apiUrl)}.`
+    )
+    process.exitCode = 1
+    return
+  }
+
   const tsServer = Date.now()
   process.stdout.write(c.dim(c.italic('Starting Web Server...\n')))
-  const apiUrl = getConfig().web.apiUrl
   // Construct the graphql url from apiUrl by default
   // But if apiGraphQLUrl is specified, use that instead
   const graphqlEndpoint = coerceRootPath(
@@ -172,3 +187,13 @@ function coerceRootPath(path: string) {
 
   return `${prefix}${path}${suffix}`
 }
+
+function isFullyQualifiedUrl(url: string) {
+  try {
+    // eslint-disable-next-line no-new
+    new URL(url)
+    return true
+  } catch (e) {
+    return false
+  }
+}

packages/api-server/src/index.ts

@@ -1,4 +1,5 @@
 #!/usr/bin/env node
+
 import { hideBin } from 'yargs/helpers'
 import yargs from 'yargs/yargs'
 
@@ -13,29 +14,38 @@ import {
 
 export * from './types'
 
-const positionalArgs = yargs(hideBin(process.argv)).parseSync()._
-
-// "bin": {
-//   "rw-api-server-watch": "./dist/watch.js",
-//   "rw-log-formatter": "./dist/logFormatter/bin.js",
-//   "rw-server": "./dist/index.js"
-// },
-
 if (require.main === module) {
-  if (positionalArgs.includes('api') && !positionalArgs.includes('web')) {
-    apiServerHandler(
-      yargs(hideBin(process.argv)).options(apiCliOptions).parseSync()
+  yargs(hideBin(process.argv))
+    .scriptName('rw-server')
+    .usage('usage: $0 <side>')
+    .strict()
+
+    .command(
+      '$0',
+      'Run both api and web servers',
+      // @ts-expect-error just passing yargs though
+      (yargs) => {
+        yargs.options(commonOptions)
+      },
+      bothServerHandler
     )
-  } else if (
-    positionalArgs.includes('web') &&
-    !positionalArgs.includes('api')
-  ) {
-    webServerHandler(
-      yargs(hideBin(process.argv)).options(webCliOptions).parseSync()
+    .command(
+      'api',
+      'Start server for serving only the api',
+      // @ts-expect-error just passing yargs though
+      (yargs) => {
+        yargs.options(apiCliOptions)
+      },
+      apiServerHandler
     )
-  } else {
-    bothServerHandler(
-      yargs(hideBin(process.argv)).options(commonOptions).parseSync()
+    .command(
+      'web',
+      'Start server for serving only the web side',
+      // @ts-expect-error just passing yargs though
+      (yargs) => {
+        yargs.options(webCliOptions)
+      },
+      webServerHandler
     )
-  }
+    .parse()
 }

packages/cli/src/commands/serve.js

@@ -122,7 +122,7 @@ export const builder = async (yargs) => {
           apiHost: {
             alias: 'api-host',
             type: 'string',
-            desc: 'Forward requests from the apiUrl, defined in redwood.toml to this host',
+            desc: 'Forward requests from the apiUrl, defined in redwood.toml, to this host',
           },
         }),
       handler: async (argv) => {

packages/cli/src/index.js

@@ -217,10 +217,20 @@ async function runYargs() {
   await loadPlugins(yarg)
 
   // Run
-  await yarg.parse(process.argv.slice(2), {}, (_err, _argv, output) => {
+  await yarg.parse(process.argv.slice(2), {}, (err, _argv, output) => {
+    // Configuring yargs with `strict` makes it error on unknown args;
+    // here we're signaling that with an exit code.
+    if (err) {
+      process.exitCode = 1
+    }
+
     // Show the output that yargs was going to if there was no callback provided
     if (output) {
-      console.log(output)
+      if (err) {
+        console.error(output)
+      } else {
+        console.log(output)
+      }
     }
   })
 }

packages/web-server/package.json

@@ -34,10 +34,9 @@
     "dotenv-defaults": "5.0.2",
     "fast-glob": "3.3.2",
     "fastify": "4.24.3",
-    "yargs-parser": "21.1.1"
+    "yargs": "17.7.2"
   },
   "devDependencies": {
-    "@types/yargs-parser": "21.0.3",
     "esbuild": "0.19.9",
     "typescript": "5.3.3"
   },

packages/web-server/src/server.ts

@@ -5,19 +5,14 @@ import path from 'path'
 import chalk from 'chalk'
 import { config } from 'dotenv-defaults'
 import Fastify from 'fastify'
-import yargsParser from 'yargs-parser'
+import { hideBin } from 'yargs/helpers'
+import yargs from 'yargs/yargs'
 
 import { getPaths, getConfig } from '@redwoodjs/project-config'
 
 import { redwoodFastifyWeb } from './web'
 import { withApiProxy } from './withApiProxy'
 
-interface Opts {
-  socket?: string
-  port?: string
-  apiHost?: string
-}
-
 function isFullyQualifiedUrl(url: string) {
   try {
     // eslint-disable-next-line no-new
@@ -29,22 +24,29 @@ function isFullyQualifiedUrl(url: string) {
 }
 
 async function serve() {
-  // Parse server file args
-  const args = yargsParser(process.argv.slice(2), {
-    string: ['port', 'socket', 'apiHost'],
-    alias: { apiHost: ['api-host'], port: ['p'] },
-  })
-
-  const options: Opts = {
-    socket: args.socket,
-    port: args.port,
-    apiHost: args.apiHost,
-  }
+  const options = yargs(hideBin(process.argv))
+    .scriptName('rw-web-server')
+    .usage('$0', 'Start server for serving only the web side')
+    .strict()
+
+    .options({
+      port: {
+        default: getConfig().web?.port || 8910,
+        type: 'number',
+        alias: 'p',
+      },
+      socket: { type: 'string' },
+      apiHost: {
+        alias: 'api-host',
+        type: 'string',
+        desc: 'Forward requests from the apiUrl, defined in redwood.toml, to this host',
+      },
+    })
+    .parseSync()
 
   const redwoodProjectPaths = getPaths()
   const redwoodConfig = getConfig()
 
-  const port = options.port ? parseInt(options.port) : redwoodConfig.web.port
   const apiUrl = redwoodConfig.web.apiUrl
 
   if (!options.apiHost && !isFullyQualifiedUrl(apiUrl)) {
@@ -110,7 +112,7 @@ async function serve() {
     listenOptions = { path: options.socket }
   } else {
     listenOptions = {
-      port,
+      port: options.port,
       host: process.env.NODE_ENV === 'production' ? '0.0.0.0' : '::',
     }
   }
@@ -121,7 +123,7 @@ async function serve() {
     if (options.socket) {
       console.log(`Web server started on ${options.socket}`)
     } else {
-      console.log(`Web server started on http://localhost:${port}`)
+      console.log(`Web server started on http://localhost:${options.port}`)
     }
   })
 

tasks/server-tests/__snapshots__/server.test.mjs.snap

@@ -0,0 +1,51 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`serve web (/Users/dom/projects/redwood/redwood/packages/web-server/dist/server.js) errors out on unknown args 1`] = `
+"rw-web-server
+
+Start server for serving only the web side
+
+Options:
+      --help                 Show help                                 [boolean]
+      --version              Show version number                       [boolean]
+  -p, --port                                            [number] [default: 8910]
+      --socket                                                          [string]
+      --apiHost, --api-host  Forward requests from the apiUrl, defined in
+                             redwood.toml, to this host                 [string]
+
+Unknown arguments: foo, bar, baz
+"
+`;
+
+exports[`serve web (/Users/dom/projects/redwood/redwood/packages/web-server/dist/server.js) fails if apiHost isn't set and apiUrl isn't fully qualified 1`] = `
+"Error: If you don't provide apiHost, apiUrl needs to be a fully-qualified URL. But apiUrl is /.redwood/functions.
+"
+`;
+
+exports[`serve web ([
+  '/Users/dom/projects/redwood/redwood/packages/api-server/dist/index.js',
+  'web'
+]) errors out on unknown args 1`] = `
+"rw-server web
+
+Start server for serving only the web side
+
+Options:
+      --help                 Show help                                 [boolean]
+      --version              Show version number                       [boolean]
+  -p, --port                                            [number] [default: 8910]
+      --socket                                                          [string]
+      --apiHost, --api-host  Forward requests from the apiUrl, defined in
+                             redwood.toml, to this host                 [string]
+
+Unknown arguments: foo, bar, baz
+"
+`;
+
+exports[`serve web ([
+  '/Users/dom/projects/redwood/redwood/packages/api-server/dist/index.js',
+  'web'
+]) fails if apiHost isn't set and apiUrl isn't fully qualified 1`] = `
+"Error: If you don't provide apiHost, apiUrl needs to be a fully-qualified URL. But apiUrl is /.redwood/functions.
+"
+`;

tasks/server-tests/jest.config.js

@@ -1,7 +1,9 @@
 /** @type {import('jest').Config} */
 const config = {
   rootDir: '.',
+  testMatch: ['<rootDir>/*.test.mjs'],
   testTimeout: 5_000 * 2,
+  transform: {},
 }
 
 module.exports = config