feat(dev): enable the usage of the command property without port (#3662)

* feat(dev): enable the usage of the command property without port

* chore: fix tests

Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>

Author: Lukas Holzer

src/commands/dev/index.js

@@ -1,3 +1,4 @@
+// @ts-check
 const path = require('path')
 const process = require('process')
 const { promisify } = require('util')
@@ -53,53 +54,75 @@ const isNonExistingCommandError = ({ command, error }) => {
   )
 }
 
-const startFrameworkServer = async function ({ settings }) {
-  if (settings.useStaticServer) {
-    return await startStaticServer({ settings })
-  }
-
-  log(`${NETLIFYDEVLOG} Starting Netlify Dev with ${settings.framework || 'custom config'}`)
-
-  // we use reject=false to avoid rejecting synchronously when the command doesn't exist
-  const frameworkProcess = execa.command(settings.command, {
+/**
+ * Run a command and pipe stdout, stderr and stdin
+ * @param {string} command
+ * @param {NodeJS.ProcessEnv} env
+ * @returns {execa.ExecaChildProcess<string>}
+ */
+const runCommand = (command, env = {}) => {
+  const commandProcess = execa.command(command, {
     preferLocal: true,
+    // we use reject=false to avoid rejecting synchronously when the command doesn't exist
     reject: false,
-    env: settings.env,
+    env,
     // windowsHide needs to be false for child process to terminate properly on Windows
     windowsHide: false,
   })
-  frameworkProcess.stdout.pipe(stripAnsiCc.stream()).pipe(process.stdout)
-  frameworkProcess.stderr.pipe(stripAnsiCc.stream()).pipe(process.stderr)
-  process.stdin.pipe(frameworkProcess.stdin)
+
+  commandProcess.stdout.pipe(stripAnsiCc.stream()).pipe(process.stdout)
+  commandProcess.stderr.pipe(stripAnsiCc.stream()).pipe(process.stderr)
+  process.stdin.pipe(commandProcess.stdin)
 
   // we can't try->await->catch since we don't want to block on the framework server which
   // is a long running process
   // eslint-disable-next-line promise/catch-or-return,promise/prefer-await-to-then
-  frameworkProcess.then(async () => {
-    const result = await frameworkProcess
-    const [commandWithoutArgs] = settings.command.split(' ')
+  commandProcess.then(async () => {
+    const result = await commandProcess
+    const [commandWithoutArgs] = command.split(' ')
     // eslint-disable-next-line promise/always-return
     if (result.failed && isNonExistingCommandError({ command: commandWithoutArgs, error: result })) {
       log(
         NETLIFYDEVERR,
-        `Failed launching framework server. Please verify ${chalk.magenta(`'${commandWithoutArgs}'`)} exists`,
+        `Failed running command: ${command}. Please verify ${chalk.magenta(`'${commandWithoutArgs}'`)} exists`,
       )
     } else {
       const errorMessage = result.failed
         ? `${NETLIFYDEVERR} ${result.shortMessage}`
-        : `${NETLIFYDEVWARN} "${settings.command}" exited with code ${result.exitCode}`
+        : `${NETLIFYDEVWARN} "${command}" exited with code ${result.exitCode}`
 
       log(`${errorMessage}. Shutting down Netlify Dev server`)
     }
     process.exit(1)
   })
   ;['SIGINT', 'SIGTERM', 'SIGQUIT', 'SIGHUP', 'exit'].forEach((signal) => {
     process.on(signal, () => {
-      frameworkProcess.kill('SIGTERM', { forceKillAfterTimeout: 500 })
+      commandProcess.kill('SIGTERM', { forceKillAfterTimeout: 500 })
       process.exit()
     })
   })
 
+  return commandProcess
+}
+
+/**
+ * Start a static server if the `useStaticServer` is provided or a framework specific server
+ * @param {object} config
+ * @param {import('../../utils/types').ServerSettings} config.settings
+ * @returns {Promise<void>}
+ */
+const startFrameworkServer = async function ({ settings }) {
+  if (settings.useStaticServer) {
+    if (settings.command) {
+      runCommand(settings.command, settings.env)
+    }
+    return await startStaticServer({ settings })
+  }
+
+  log(`${NETLIFYDEVLOG} Starting Netlify Dev with ${settings.framework || 'custom config'}`)
+
+  runCommand(settings.command, settings.env)
+
   try {
     const open = await waitPort({
       port: settings.frameworkPort,
@@ -185,6 +208,7 @@ class DevCommand extends Command {
     const { api, config, site, siteInfo } = this.netlify
     config.dev = { ...config.dev }
     config.build = { ...config.build }
+    /** @type {import('./types').DevConfig} */
     const devConfig = {
       framework: '#auto',
       ...(config.functionsDirectory && { functions: config.functionsDirectory }),
@@ -207,6 +231,7 @@ class DevCommand extends Command {
       siteInfo,
     })
 
+    /** @type {Partial<import('../../utils/types').ServerSettings>} */
     let settings = {}
     try {
       settings = await detectServerSettings(devConfig, flags, site.root)

src/commands/dev/types.d.ts

@@ -0,0 +1,21 @@
+import type { FrameworkNames } from '../../utils/types';
+
+export type DevConfig = {
+  framework: FrameworkNames
+  /** Directory of the functions */
+  functions: string
+  publish: string
+  /** Port to serve the functions */
+  port: number
+  live: boolean
+  staticServerPort?: number
+  targetPort?: number
+  /** Command to run */
+  command?: string
+  functionsPort?: number
+  autoLaunch?: boolean
+  https?: {
+    keyFile: string
+    certFile: string
+  }
+}

src/utils/detect-server-settings.js

@@ -1,3 +1,4 @@
+// @ts-check
 const { EOL } = require('os')
 const path = require('path')
 const process = require('process')
@@ -101,6 +102,14 @@ const getDefaultDist = () => {
   return process.cwd()
 }
 
+/**
+ *
+ * @param {object} param0
+ * @param {import('../commands/dev/types').DevConfig} param0.devConfig
+ * @param {Record<string, string>} param0.flags
+ * @param {string} param0.projectDir
+ * @returns {Promise<import('./types').BaseServerSettings>}
+ */
 const handleStaticServer = async ({ devConfig, flags, projectDir }) => {
   validateNumberProperty({ devConfig, property: 'staticServerPort' })
 
@@ -114,14 +123,6 @@ const handleStaticServer = async ({ devConfig, flags, projectDir }) => {
     )
   }
 
-  if (devConfig.command) {
-    log(
-      `${NETLIFYDEVWARN} Ignoring command setting since using a simple static server. Configure ${formatProperty(
-        'command',
-      )} ${chalk.bold('and')} ${formatProperty('targetPort')} for a custom setup`,
-    )
-  }
-
   if (devConfig.targetPort) {
     log(
       `${NETLIFYDEVWARN} Ignoring ${formatProperty(
@@ -139,12 +140,18 @@ const handleStaticServer = async ({ devConfig, flags, projectDir }) => {
     errorMessage: 'Could not acquire configured static server port',
   })
   return {
+    ...(devConfig.command && { command: devConfig.command }),
     useStaticServer: true,
     frameworkPort,
     dist,
   }
 }
 
+/**
+ * Retrieves the settings from a framework
+ * @param {import('./types').FrameworkInfo} framework
+ * @returns {import('./types').BaseServerSettings}
+ */
 const getSettingsFromFramework = (framework) => {
   const {
     build: { directory: dist },
@@ -211,6 +218,11 @@ const detectFrameworkSettings = async ({ projectDir }) => {
 
 const hasCommandAndTargetPort = ({ devConfig }) => devConfig.command && devConfig.targetPort
 
+/**
+ * Creates settings for the custom framework
+ * @param {*} param0
+ * @returns {import('./types').BaseServerSettings}
+ */
 const handleCustomFramework = ({ devConfig }) => {
   if (!hasCommandAndTargetPort({ devConfig })) {
     throw new Error(
@@ -228,6 +240,11 @@ const handleCustomFramework = ({ devConfig }) => {
   }
 }
 
+/**
+ * Handles a forced framework and retrieves the settings for it
+ * @param {*} param0
+ * @returns {Promise<import('./types').BaseServerSettings>}
+ */
 const handleForcedFramework = async ({ devConfig, projectDir }) => {
   // this throws if `devConfig.framework` is not a supported framework
   const { command, dist, env, framework, frameworkPort, pollingStrategies } = getSettingsFromFramework(
@@ -243,9 +260,17 @@ const handleForcedFramework = async ({ devConfig, projectDir }) => {
   }
 }
 
+/**
+ * Get the server settings based on the flags and the devConfig
+ * @param {import('../commands/dev/types').DevConfig} devConfig
+ * @param {Record<string, string>} flags
+ * @param {string} projectDir
+ * @returns {Promise<import('./types').ServerSettings>}
+ */
 const detectServerSettings = async (devConfig, flags, projectDir) => {
   validateStringProperty({ devConfig, property: 'framework' })
 
+  /** @type {Partial<import('./types').BaseServerSettings>} */
   let settings = {}
 
   if (flags.dir || devConfig.framework === '#static') {
@@ -254,7 +279,6 @@ const detectServerSettings = async (devConfig, flags, projectDir) => {
   } else if (devConfig.framework === '#auto') {
     // this is the default CLI behavior
 
-    // we don't need to run the detection if both command and targetPort are configured
     const runDetection = !hasCommandAndTargetPort({ devConfig })
     const frameworkSettings = runDetection ? await detectFrameworkSettings({ projectDir }) : undefined
 

src/utils/types.d.ts

@@ -0,0 +1,44 @@
+export type FrameworkNames = '#static' | '#auto' | '#custom' | string
+
+export type FrameworkInfo = {
+  build: {
+    directory: string
+  }
+  dev: {
+    commands: string[]
+    port: number
+    pollingStrategies: { name: string }[]
+  }
+  name: FrameworkNames
+  staticAssetsDirectory: string
+  env: NodeJS.ProcessEnv
+}
+
+export type BaseServerSettings = {
+  dist: string
+
+  // static serving
+  useStaticServer?: true
+
+  // Framework specific part
+  /** A port where a proxy can listen to it */
+  frameworkPort?: number
+  functions?: string
+  /** The command that was provided for the dev config */
+  command?: string
+  /** The framework name ('Create React App') */
+  framework?: string
+  env?: NodeJS.ProcessEnv
+  pollingStrategies?: string[]
+}
+
+export type ServerSettings = BaseServerSettings & {
+  /** default 'secret' */
+  jwtSecret: string
+  /** default 'app_metadata.authorization.roles' */
+  jwtRolePath: string
+  /** The port where the functions are running on */
+  port: number
+  /** The directory of the functions */
+  functions: number
+}

tests/framework-detection.test.js

@@ -65,7 +65,7 @@ test('should use static server when framework is set to #static', async (t) => {
   })
 })
 
-test('should warn if using static server and `command` is configured', async (t) => {
+test('should log the command if using static server and `command` is configured', async (t) => {
   await withSiteBuilder('site-with-index-file', async (builder) => {
     await builder
       .withContentFile({

tests/snapshots/framework-detection.test.js.md

@@ -64,13 +64,12 @@ Generated by [AVA](https://avajs.dev).
     ␊
     `
 
-## should warn if using static server and `command` is configured
+## should log the command if using static server and `command` is configured
 
 > Snapshot 1
 
     `◈ Netlify Dev ◈␊
     ◈ Using simple static server because '--dir' flag was specified␊
-    ◈ Ignoring command setting since using a simple static server. Configure 'command' and 'targetPort' for a custom setup␊
     ◈ Running static server from "site-with-index-file/public"␊
     ␊
     ◈ Static server listening to 88888␊
@@ -118,7 +117,7 @@ Generated by [AVA](https://avajs.dev).
 
     `◈ Netlify Dev ◈␊
     ◈ Starting Netlify Dev with Create React App␊
-    ◈ Failed launching framework server. Please verify 'react-scripts' exists`
+    ◈ Failed running command: react-scripts start. Please verify 'react-scripts' exists`
 
 ## should throw when forcing a non supported framework
 
@@ -171,7 +170,7 @@ Generated by [AVA](https://avajs.dev).
     ◈ Setup a netlify.toml file with a [dev] section to specify your dev server settings.␊
     ◈ See docs at: https://cli.netlify.com/netlify-dev#project-detection␊
     ◈ Starting Netlify Dev with #custom␊
-    ◈ Failed launching framework server. Please verify 'oops-i-did-it-again' exists`
+    ◈ Failed running command: oops-i-did-it-again forgot-to-use-a-valid-command. Please verify 'oops-i-did-it-again' exists`
 
 ## should prompt when multiple frameworks are detected