IOEXT-2183: fix: reload config after pre-app hooks and default --web-optimize from NODE_ENV (CEXT-6815)

src/commands/app/build.js

@@ -15,7 +15,7 @@ const chalk = require('chalk')
 
 const BaseCommand = require('../../BaseCommand')
 const { Flags } = require('@oclif/core')
-const { runInProcess, writeConfig } = require('../../lib/app-helper')
+const { runInProcess, writeConfig, reloadEnvOnHooks } = require('../../lib/app-helper')
 const RuntimeLib = require('@adobe/aio-lib-runtime')
 const { bundle } = require('@adobe/aio-lib-web')
 const fs = require('fs-extra')
@@ -68,6 +68,7 @@ class Build extends BaseCommand {
     } catch (err) {
       this.log(err)
     }
+    reloadEnvOnHooks()
 
     if (flags.actions) {
       // removed flags['force-build'] || as it is always forced at this point, we need to check to decide what to build
@@ -117,7 +118,7 @@ class Build extends BaseCommand {
             const bundleOptions = {
               shouldDisableCache: true,
               shouldContentHash: flags['content-hash'],
-              shouldOptimize: flags['web-optimize'],
+              shouldOptimize: flags['web-optimize'] ?? process.env.NODE_ENV === 'production',
               logLevel: flags.verbose ? 'verbose' : 'warn'
             }
             // empty the dist folder to prevent an S3 explosion
@@ -179,8 +180,8 @@ Build.flags = {
     allowNo: true
   }),
   'web-optimize': Flags.boolean({
-    description: '[default: false] Enable optimization (minification) of js/css/html',
-    default: false
+    description: '[default: true if NODE_ENV=production, false otherwise] Enable optimization (minification) of js/css/html',
+    allowNo: true
   }),
   extension: Flags.string({
     description: 'Build only a specific extension point, the flags can be specified multiple times',

src/commands/app/deploy.js

@@ -19,7 +19,7 @@ const BuildCommand = require('./build')
 const webLib = require('@adobe/aio-lib-web')
 const { Flags } = require('@oclif/core')
 const {
-  rewriteActionUrlInEntities, runInProcess,
+  rewriteActionUrlInEntities, runInProcess, reloadEnvOnHooks,
   buildExtensionPointPayloadWoMetadata, buildExcShellViewExtensionMetadata,
   getFilesCountWithExtension
 } = require('../../lib/app-helper')
@@ -206,6 +206,7 @@ class Deploy extends BuildCommand {
     } catch (err) {
       this.error(err)
     }
+    reloadEnvOnHooks()
 
     // provision database if configured
     if (config.manifest?.full?.database?.['auto-provision'] === true) {
@@ -487,8 +488,8 @@ Deploy.flags = {
     exclusive: ['action', 'publish'] // no-publish is excluded
   }),
   'web-optimize': Flags.boolean({
-    description: '[default: false] Enable optimization (minification) of web js/css/html',
-    default: false
+    description: '[default: true if NODE_ENV=production, false otherwise] Enable optimization (minification) of web js/css/html',
+    allowNo: true
   }),
   'log-forwarding-update': Flags.boolean({
     description: '[default: true] Update log forwarding configuration on server',

src/commands/app/run.js

@@ -23,7 +23,7 @@ const coreConfig = require('@adobe/aio-lib-core-config')
 const BaseCommand = require('../../BaseCommand')
 const runDev = require('../../lib/run-dev')
 const { defaultHttpServerPort: SERVER_DEFAULT_PORT } = require('../../lib/defaults')
-const { runInProcess } = require('../../lib/app-helper')
+const { runInProcess, reloadEnvOnHooks } = require('../../lib/app-helper')
 
 const DEV_KEYS_DIR = 'dist/dev-keys/'
 const PRIVATE_KEY_PATH = DEV_KEYS_DIR + 'private.key'
@@ -84,6 +84,7 @@ class Run extends BaseCommand {
     } catch (err) {
       this.log(err)
     }
+    reloadEnvOnHooks()
 
     // check if there are certificates available, and generate them if not ...
     // only care about certificates if the application has a UI

src/commands/app/undeploy.js

@@ -17,7 +17,7 @@ const { Flags } = require('@oclif/core')
 
 const BaseCommand = require('../../BaseCommand')
 const webLib = require('@adobe/aio-lib-web')
-const { runInProcess, buildExtensionPointPayloadWoMetadata } = require('../../lib/app-helper')
+const { runInProcess, buildExtensionPointPayloadWoMetadata, reloadEnvOnHooks } = require('../../lib/app-helper')
 const rtLib = require('@adobe/aio-lib-runtime')
 const { sendAppAssetsUndeployedAuditLog, sendAppUndeployAuditLog } = require('../../lib/audit-logger')
 const { setRuntimeApiHostAndAuthHandler, getAccessToken } = require('../../lib/auth-helper')
@@ -136,6 +136,7 @@ class Undeploy extends BaseCommand {
     } catch (err) {
       this.log(err)
     }
+    reloadEnvOnHooks()
 
     if (flags.actions) {
       if (config.app.hasBackend) {

src/lib/app-helper.js

@@ -81,6 +81,23 @@ async function runPackageScript (scriptName, dir, cmdArgs = []) {
   }
 }
 
+/**
+ * Reloads the aio config so that changes made by a hook to .env or .aio are picked up.
+ * Opt-in: it only reloads when the AIO_RELOAD_ENV_ON_HOOKS env var is set to a truthy value.
+ *
+ * @returns {boolean} true if the config was reloaded
+ */
+function reloadEnvOnHooks () {
+  const value = (process.env.AIO_RELOAD_ENV_ON_HOOKS || '').trim().toLowerCase()
+  if (value === '' || value === '0' || value === 'false') {
+    aioLogger.debug('reloadEnvOnHooks: AIO_RELOAD_ENV_ON_HOOKS is not set, skipping config reload')
+    return false
+  }
+  aioLogger.debug('reloadEnvOnHooks: AIO_RELOAD_ENV_ON_HOOKS is set, reloading config')
+  aioConfig.reload()
+  return true
+}
+
 /**
  * @param {string} hookPath to be require()'d and run. Should export an async function that takes a config object as its only argument
  * @param {object} config which will be passed to the hook
@@ -557,6 +574,7 @@ module.exports = {
   installPackages,
   runScript,
   runInProcess,
+  reloadEnvOnHooks,
   runPackageScript,
   wrapError,
   removeProtocolFromURL,

src/lib/build-actions.js

@@ -10,7 +10,7 @@ OF ANY KIND, either express or implied. See the License for the specific languag
 governing permissions and limitations under the License.
 */
 
-const { runInProcess } = require('./app-helper')
+const { runInProcess, reloadEnvOnHooks } = require('./app-helper')
 const { buildActions } = require('@adobe/aio-lib-runtime')
 
 /**
@@ -22,6 +22,7 @@ const { buildActions } = require('@adobe/aio-lib-runtime')
  */
 module.exports = async (config, filterActions, forceBuild = false) => {
   await runInProcess(config.hooks['pre-app-build'], config)
+  reloadEnvOnHooks()
   const script = await runInProcess(config.hooks['build-actions'], { config, options: { filterActions, forceBuild } })
   if (!script) {
     await buildActions(config, filterActions, forceBuild)

src/lib/deploy-actions.js

@@ -10,7 +10,7 @@ OF ANY KIND, either express or implied. See the License for the specific languag
 governing permissions and limitations under the License.
 */
 
-const { runInProcess } = require('./app-helper')
+const { runInProcess, reloadEnvOnHooks } = require('./app-helper')
 const { deployActions } = require('@adobe/aio-lib-runtime')
 const logActions = require('./log-actions')
 
@@ -31,6 +31,7 @@ module.exports = async ({
   inprocHook
 }) => {
   await runInProcess(config.hooks['pre-app-deploy'], config)
+  reloadEnvOnHooks()
 
   const hookFilterEntities = Array.isArray(deployConfig.filterEntities?.actions) ? deployConfig.filterEntities.actions : []
   const hookData = {

test/commands/app/build.test.js

@@ -255,6 +255,37 @@ describe('run', () => {
     expect(mockBundleFunc).toHaveBeenCalledTimes(1)
   })
 
+  test('reloads env after pre-app-build and before building', async () => {
+    helpers.reloadEnvOnHooks.mockClear()
+    command.getAppExtConfigs.mockResolvedValueOnce(createAppConfig(command.appConfig))
+
+    await command.run()
+    expect(helpers.reloadEnvOnHooks).toHaveBeenCalledTimes(1)
+    expect(helpers.runInProcess.mock.invocationCallOrder[0]).toBeLessThan(helpers.reloadEnvOnHooks.mock.invocationCallOrder[0])
+    expect(helpers.reloadEnvOnHooks.mock.invocationCallOrder[0]).toBeLessThan(mockRuntimeLib.buildActions.mock.invocationCallOrder[0])
+  })
+
+  test.each([
+    [[], 'production', true],
+    [[], 'development', false],
+    [[], undefined, false],
+    [['--web-optimize'], undefined, true],
+    [['--no-web-optimize'], 'production', false]
+  ])('web-optimize %j with NODE_ENV=%s optimizes: %s', async (argv, nodeEnv, expected) => {
+    const originalNodeEnv = process.env.NODE_ENV
+    if (nodeEnv) process.env.NODE_ENV = nodeEnv
+    else delete process.env.NODE_ENV
+    try {
+      command.getAppExtConfigs.mockResolvedValueOnce(createAppConfig(command.appConfig))
+      command.argv = argv
+      await command.run()
+      expect(mockWebLib.bundle).toHaveBeenCalledWith(expect.any(String), expect.any(String),
+        expect.objectContaining({ shouldOptimize: expected }), expect.any(Function))
+    } finally {
+      process.env.NODE_ENV = originalNodeEnv
+    }
+  })
+
   test('build & deploy an App with --no-content-hash', async () => {
     command.getAppExtConfigs.mockResolvedValueOnce(createAppConfig(command.appConfig))
 

test/commands/app/deploy.test.js

@@ -869,8 +869,9 @@ describe('run', () => {
     expect(command.error).toHaveBeenCalledTimes(0)
   })
 
-  test('deploy reloads config before deploying actions', async () => {
+  test('deploy reloads env after pre-app-deploy and reloads config before deploying actions', async () => {
     command.getAppExtConfigs.mockResolvedValueOnce(createAppConfig(command.appConfig))
+    helpers.reloadEnvOnHooks.mockClear()
     const noScriptFound = undefined
     helpers.runInProcess
       .mockResolvedValueOnce(noScriptFound) // pre-app-deploy
@@ -880,8 +881,13 @@ describe('run', () => {
     command.argv = ['--no-web-assets']
     await command.run()
 
-    expect(mockConfig.reload).toHaveBeenCalledTimes(1)
     expect(mockRuntimeLib.deployActions).toHaveBeenCalledTimes(1)
+    // env reload is gated, the config reload just before deploying actions is not
+    expect(helpers.reloadEnvOnHooks).toHaveBeenCalledTimes(1)
+    expect(helpers.runInProcess.mock.invocationCallOrder[0]).toBeLessThan(
+      helpers.reloadEnvOnHooks.mock.invocationCallOrder[0]
+    )
+    expect(mockConfig.reload).toHaveBeenCalledTimes(1)
     expect(mockConfig.reload.mock.invocationCallOrder[0]).toBeLessThan(
       mockRuntimeLib.deployActions.mock.invocationCallOrder[0]
     )
@@ -1368,8 +1374,7 @@ describe('run', () => {
         'log-forwarding-update': true,
         open: false,
         publish: false,
-        'web-assets': true,
-        'web-optimize': false
+        'web-assets': true
       },
       env: mockEnv,
       opItems: [
@@ -1492,8 +1497,7 @@ describe('run', () => {
         'log-forwarding-update': true,
         open: false,
         publish: false,
-        'web-assets': true,
-        'web-optimize': false
+        'web-assets': true
       },
       env: mockEnv,
       opItems: [

test/commands/app/run.test.js

@@ -701,4 +701,22 @@ describe('run', () => {
     expect(scriptSequence[0]).toEqual('pre-app-run')
     expect(scriptSequence[1]).toEqual('post-app-run')
   })
+
+  test('reloads env after pre-app-run and before starting the dev environment', async () => {
+    mockFSExists([PRIVATE_KEY_PATH, PUB_CERT_PATH])
+    helpers.reloadEnvOnHooks.mockClear()
+    command.getAppExtConfigs.mockResolvedValueOnce(createAppConfig(command.appConfig))
+
+    command.argv = []
+    await command.run()
+
+    expect(helpers.reloadEnvOnHooks).toHaveBeenCalledTimes(1)
+    expect(mockRunDev).toHaveBeenCalledTimes(1)
+    expect(helpers.runInProcess.mock.invocationCallOrder[0]).toBeLessThan(
+      helpers.reloadEnvOnHooks.mock.invocationCallOrder[0]
+    )
+    expect(helpers.reloadEnvOnHooks.mock.invocationCallOrder[0]).toBeLessThan(
+      mockRunDev.mock.invocationCallOrder[0]
+    )
+  })
 })

test/commands/app/undeploy.test.js

@@ -198,6 +198,16 @@ describe('run', () => {
     expect(mockWebLib.undeployWeb).toHaveBeenCalledTimes(1)
   })
 
+  test('reloads env after pre-app-undeploy and before undeploying', async () => {
+    helpers.reloadEnvOnHooks.mockClear()
+    command.getAppExtConfigs.mockResolvedValueOnce(createAppConfig())
+
+    await command.run()
+    expect(helpers.reloadEnvOnHooks).toHaveBeenCalledTimes(1)
+    expect(helpers.runInProcess.mock.invocationCallOrder[0]).toBeLessThan(helpers.reloadEnvOnHooks.mock.invocationCallOrder[0])
+    expect(helpers.reloadEnvOnHooks.mock.invocationCallOrder[0]).toBeLessThan(mockRuntimeLib.undeployActions.mock.invocationCallOrder[0])
+  })
+
   test('undeploy an App with no flags with hooks', async () => {
     command.getAppExtConfigs.mockResolvedValueOnce(createAppConfig())
     __setupMockHooks()

test/commands/lib/app-helper.test.js

@@ -42,6 +42,7 @@ beforeEach(() => {
   execa.command.mockReset()
   aioConfig.get.mockReset()
   aioConfig.set.mockReset()
+  aioConfig.reload.mockReset()
   libEnv.getCliEnv.mockReset()
   libIms.getToken.mockReset()
   mockFetch.mockReset()
@@ -248,6 +249,36 @@ test('runInProcess fails with no hook-path', async () => {
   expect(mockLogger.debug).toHaveBeenCalledWith('runInProcess: undefined hookPath')
 })
 
+describe('reloadEnvOnHooks', () => {
+  const OLD_VALUE = process.env.AIO_RELOAD_ENV_ON_HOOKS
+
+  afterEach(() => {
+    if (OLD_VALUE === undefined) {
+      delete process.env.AIO_RELOAD_ENV_ON_HOOKS
+    } else {
+      process.env.AIO_RELOAD_ENV_ON_HOOKS = OLD_VALUE
+    }
+  })
+
+  test('does not reload when AIO_RELOAD_ENV_ON_HOOKS is not set', () => {
+    delete process.env.AIO_RELOAD_ENV_ON_HOOKS
+    expect(appHelper.reloadEnvOnHooks()).toBe(false)
+    expect(aioConfig.reload).not.toHaveBeenCalled()
+  })
+
+  test.each(['', '  ', '0', 'false', 'FALSE'])('does not reload when AIO_RELOAD_ENV_ON_HOOKS is %p', (value) => {
+    process.env.AIO_RELOAD_ENV_ON_HOOKS = value
+    expect(appHelper.reloadEnvOnHooks()).toBe(false)
+    expect(aioConfig.reload).not.toHaveBeenCalled()
+  })
+
+  test.each(['1', 'true', 'TRUE', 'yes'])('reloads when AIO_RELOAD_ENV_ON_HOOKS is %p', (value) => {
+    process.env.AIO_RELOAD_ENV_ON_HOOKS = value
+    expect(appHelper.reloadEnvOnHooks()).toBe(true)
+    expect(aioConfig.reload).toHaveBeenCalledTimes(1)
+  })
+})
+
 test('runScript with empty command', async () => {
   await appHelper.runScript(undefined, 'dir')
   expect(execa.command).not.toHaveBeenCalled()

test/commands/lib/build-actions.test.js

@@ -28,6 +28,15 @@ beforeEach(() => {
   utils.runInProcess.mockReset()
 })
 
+test('reloads env after pre-app-build', async () => {
+  utils.reloadEnvOnHooks.mockClear()
+  utils.runInProcess.mockImplementation(script => script === 'build-actions' ? {} : undefined)
+  await buildActions(extensionConfig)
+  expect(utils.reloadEnvOnHooks).toHaveBeenCalledTimes(1)
+  expect(utils.runInProcess.mock.invocationCallOrder[0]).toBeLessThan(utils.reloadEnvOnHooks.mock.invocationCallOrder[0])
+  expect(utils.reloadEnvOnHooks.mock.invocationCallOrder[0]).toBeLessThan(utils.runInProcess.mock.invocationCallOrder[1])
+})
+
 test('exports', () => {
   expect(typeof buildActions).toEqual('function')
 })

test/commands/lib/deploy-actions.test.js

@@ -35,6 +35,14 @@ test('exports', () => {
   expect(typeof deployActions).toEqual('function')
 })
 
+test('reloads env after pre-app-deploy', async () => {
+  utils.reloadEnvOnHooks.mockClear()
+  await deployActions({ config: { hooks: {} } })
+  expect(utils.reloadEnvOnHooks).toHaveBeenCalledTimes(1)
+  expect(utils.runInProcess.mock.invocationCallOrder[0]).toBeLessThan(utils.reloadEnvOnHooks.mock.invocationCallOrder[0])
+  expect(utils.reloadEnvOnHooks.mock.invocationCallOrder[0]).toBeLessThan(rtDeployActions.mock.invocationCallOrder[0])
+})
+
 test('deploy-actions app hook available', async () => {
   utils.runInProcess.mockImplementation(script => {
     if (script === 'deploy-actions') {