Optimize build and deploy (#811)

* change default --force-build to false
* handle cases where manifest specifies action dir, and it changes during run
* pass useForce for --force-deply w/tests:100%
* use aio-lib-runtime with optimized build+deploy, update docs

package.json

@@ -12,7 +12,7 @@
     "@adobe/aio-lib-core-networking": "^5",
     "@adobe/aio-lib-env": "^3",
     "@adobe/aio-lib-ims": "^7",
-    "@adobe/aio-lib-runtime": "^6",
+    "@adobe/aio-lib-runtime": "^7.0.0",
     "@adobe/aio-lib-templates": "^3",
     "@adobe/aio-lib-web": "^7",
     "@adobe/generator-aio-app": "^7",

src/commands/app/build.js

@@ -70,22 +70,24 @@ class Build extends BaseCommand {
     }
 
     if (flags.actions) {
-      if (config.app.hasBackend && (flags['force-build'] || !fs.existsSync(config.actions.dist))) {
+      // removed flags['force-build'] || as it is always forced at this point, we need to check to decide what to build
+      // if no backend, we skip the build
+      if (config.app.hasBackend) {
         try {
           let builtList = []
           const script = await runInProcess(config.hooks['build-actions'], config)
           aioLogger.debug(`run hook for 'build-actions' for actions in '${name}' returned ${script}`)
           spinner.start(`Building actions for '${name}'`)
           if (!script) {
-            builtList = await RuntimeLib.buildActions(config, filterActions, true)
+            builtList = await RuntimeLib.buildActions(config, filterActions, flags['force-build']) // skipCheck is false
           }
           if (builtList.length > 0) {
             spinner.succeed(chalk.green(`Built ${builtList.length} action(s) for '${name}'`))
           } else {
             if (script) {
               spinner.fail(chalk.green(`build-action skipped by hook '${name}'`))
             } else {
-              spinner.fail(chalk.green(`No actions built for '${name}'`))
+              spinner.info(chalk.green(`No actions built for '${name}'`))
             }
           }
           aioLogger.debug(`RuntimeLib.buildActions returned ${builtList}`)
@@ -94,7 +96,7 @@ class Build extends BaseCommand {
           throw err
         }
       } else {
-        spinner.info(`no backend or a build already exists, skipping action build for '${name}'`)
+        spinner.info(`no backend, skipping action build for '${name}'`)
       }
     }
     if (flags['web-assets']) {
@@ -127,7 +129,8 @@ class Build extends BaseCommand {
           throw err
         }
       } else {
-        spinner.info(`no frontend or a build already exists, skipping frontend build for '${name}'`)
+        // TODO: specify which ... keep this useful
+        spinner.info(chalk.green(`No frontend or a build already exists, skipping frontend build for '${name}'`))
       }
     }
     try {
@@ -140,7 +143,8 @@ class Build extends BaseCommand {
 
 Build.description = `Build an Adobe I/O App
 
-This will always force a rebuild unless --no-force-build is set.
+Build the actions and web assets for an Adobe I/O App. Build is optimized to only build what is necessary.
+Use the --force-build flag to force a build even if one already exists.
 `
 
 Build.flags = {
@@ -163,8 +167,8 @@ Build.flags = {
     allowNo: true
   }),
   'force-build': Flags.boolean({
-    description: '[default: true] Force a build even if one already exists',
-    default: true,
+    description: '[default: false] Force a build even if one already exists',
+    default: false,
     allowNo: true
   }),
   'content-hash': Flags.boolean({

src/commands/app/deploy.js

@@ -172,7 +172,7 @@ class Deploy extends BuildCommand {
               // output should be "Error : <plugin-name> : <error-message>\n" for each failure
               this.error(hookResults.failures.map(f => `${f.plugin.name} : ${f.error.message}`).join('\nError: '), { exit: 1 })
             }
-            deployedRuntimeEntities = await rtLib.deployActions(config, { filterEntities }, onProgress)
+            deployedRuntimeEntities = await rtLib.deployActions(config, { filterEntities, useForce: flags['force-deploy'] }, onProgress)
           }
 
           if (deployedRuntimeEntities.actions && deployedRuntimeEntities.actions.length > 0) {
@@ -181,7 +181,7 @@ class Deploy extends BuildCommand {
             if (script) {
               spinner.fail(chalk.green(`deploy-actions skipped by hook '${name}'`))
             } else {
-              spinner.fail(chalk.green(`No actions deployed for '${name}'`))
+              spinner.info(chalk.green(`No actions deployed for '${name}'`))
             }
           }
         } catch (err) {
@@ -287,9 +287,13 @@ class Deploy extends BuildCommand {
   }
 }
 
-Deploy.description = `Build and deploy an Adobe I/O App
+Deploy.description = `Deploy an Adobe I/O App
 
-This will always force a rebuild unless --no-force-build is set.
+Deploys the actions and web assets for an Adobe I/O App.
+This will also build any changed actions or web assets before deploying.
+Use the --force-build flag to force a build even if one already exists.
+Deploy is optimized to only deploy what is necessary. Be aware that deploying actions will overwrite any previous deployments.
+Use the --force-deploy flag to force deploy changes, regardless of production Workspace being published in Exchange.
 `
 
 Deploy.flags = {
@@ -319,7 +323,7 @@ Deploy.flags = {
   'force-build': Flags.boolean({
     description: '[default: true] Force a build even if one already exists',
     exclusive: ['no-build'], // no-build
-    default: true,
+    default: false,
     allowNo: true
   }),
   'content-hash': Flags.boolean({

src/commands/app/run.js

@@ -108,6 +108,9 @@ class Run extends BaseCommand {
     }
 
     const verboseOutput = flags.verbose || flags.local || headlessApp
+    // we should evaluate this, a lot of output just disappears in the spinner text and
+    // using verbose dumps ALL of parcel's output, so this become unreadable
+    // we need a middle ground. -jm
     const onProgress = !verboseOutput
       ? info => {
         spinner.text = info

src/lib/actions-watcher.js

@@ -38,7 +38,7 @@ const deployActions = require('./deploy-actions')
 module.exports = async (watcherOptions) => {
   const { config, log } = watcherOptions
 
-  log(`watching action files at ${config.actions.src}...`)
+  log(`watching action files at ${config.actions.src} ...`)
   const watcher = chokidar.watch(config.actions.src)
 
   watcher.on('change', createChangeHandler({ ...watcherOptions, watcher }))
@@ -91,6 +91,8 @@ function createChangeHandler (watcherOptions) {
     try {
       aioLogger.debug(`${filePath} has changed. Redeploying actions.`)
       const filterActions = getActionNameFromPath(filePath, watcherOptions)
+      // this is happening, but its not showing up because verbose is usually off
+      // this might be more important and worthy of signalling to the user
       if (!filterActions.length) {
         log('  -> A non-action file was changed, restart is required to deploy...')
       } else {
@@ -119,14 +121,24 @@ function createChangeHandler (watcherOptions) {
  * @returns {Array<string>}  All of the actions which match the modified path
  */
 function getActionNameFromPath (filePath, watcherOptions) {
+  // note: this check only happens during aio app run
+  // before the watcher is started, all actions are built and deployed and hashes updated
+  // this code is missing 2 cases:
+  // 1. if the action is a folder with a package.json and the changed file is in the folder
+  // 2. if the changed file is in the folder with the action, but not the action file itself
+  // we need to be careful with these cases, because we could cause a recursive loop
+  // for now we continue to error on the cautious side, and output a message suggesting a restart
   const actionNames = []
   const unixFilePath = upath.toUnix(filePath)
   const { config } = watcherOptions
   Object.entries(config.manifest.full.packages).forEach(([, pkg]) => {
     if (pkg.actions) {
       Object.entries(pkg.actions).forEach(([actionName, action]) => {
         const unixActionFunction = upath.toUnix(action.function)
-        if (unixActionFunction.includes(unixFilePath)) {
+        // since action could be a folder, and changed file could be in the folder
+        // we need to compare both ways
+        // there are 2 additional cases listed above
+        if (unixActionFunction.includes(unixFilePath) || unixFilePath.includes(unixActionFunction)) {
           actionNames.push(actionName)
         }
       })

src/lib/run-dev.js

@@ -82,8 +82,7 @@ async function runDev (config, dataDir, options = {}, log = () => {}, inprocHook
 
       // build and deploy actions
       log('building actions..')
-      await buildActions(devConfig, null, true /* force build */)
-
+      await buildActions(devConfig, null, false /* force build */)
       const { cleanup: watcherCleanup } = await actionsWatcher({ config: devConfig, isLocal, log, inprocHook })
       cleanup.add(() => watcherCleanup(), 'stopping action watcher...')
     }
@@ -152,10 +151,16 @@ async function runDev (config, dataDir, options = {}, log = () => {}, inprocHook
       devConfig.app.hasFrontend = false
     }
 
-    log('setting up vscode debug configuration files...')
-    const vscodeConfig = vscode(devConfig)
-    await vscodeConfig.update({ frontEndUrl })
-    cleanup.add(() => vscodeConfig.cleanup(), 'cleaning up vscode debug configuration files...')
+    // todo: remove vscode config swapping, dev command uses a persistent file so we don't need this.
+    // also there was a latent issue with projects that defined an action src as a folder with an index.js file.
+    // it looks explicitly for package.json and fails if it does not find it.
+    // regarless, we don't need it, and when we actually remove --local we can be rid of this.
+    if (isLocal) {
+      log('setting up vscode debug configuration files...')
+      const vscodeConfig = vscode(devConfig)
+      await vscodeConfig.update({ frontEndUrl })
+      cleanup.add(() => vscodeConfig.cleanup(), 'cleaning up vscode debug configuration files...')
+    }
 
     // automatically fetch logs if there are actions
     if (config.app.hasBackend && fetchLogs) {

test/commands/app/build.test.js

@@ -182,7 +182,7 @@ test('flags', async () => {
 
   expect(typeof TheCommand.flags['force-build']).toBe('object')
   expect(typeof TheCommand.flags['force-build'].description).toBe('string')
-  expect(TheCommand.flags['force-build'].default).toEqual(true)
+  expect(TheCommand.flags['force-build'].default).toEqual(false)
   expect(TheCommand.flags['force-build'].allowNo).toEqual(true)
 
   expect(typeof TheCommand.flags['content-hash']).toBe('object')
@@ -285,6 +285,7 @@ describe('run', () => {
   })
 
   test('build & deploy an App with no force-build but build exists', async () => {
+    // build actions is now called so it can verify if the src has changed since the last build
     command.appConfig.actions.dist = 'actions'
     command.appConfig.web.distProd = 'dist'
     command.getAppExtConfigs.mockResolvedValueOnce(createAppConfig(command.appConfig))
@@ -293,7 +294,7 @@ describe('run', () => {
     mockFS.existsSync.mockReturnValue(true)
     await command.run()
     expect(command.error).toHaveBeenCalledTimes(0)
-    expect(mockRuntimeLib.buildActions).toHaveBeenCalledTimes(0)
+    expect(mockRuntimeLib.buildActions).toHaveBeenCalledTimes(1)
     expect(mockWebLib.bundle).toHaveBeenCalledTimes(0)
   })
 
@@ -326,7 +327,7 @@ describe('run', () => {
     expect(spinner.succeed).toHaveBeenCalledWith(expect.stringContaining('Built 3 action(s) for \'application\''))
     expect(command.error).toHaveBeenCalledTimes(0)
     expect(mockRuntimeLib.buildActions).toHaveBeenCalledTimes(1)
-    expect(mockRuntimeLib.buildActions).toHaveBeenCalledWith(appConfig.application, ['a', 'b', 'c'], true)
+    expect(mockRuntimeLib.buildActions).toHaveBeenCalledWith(appConfig.application, ['a', 'b', 'c'], false)
     expect(mockWebLib.bundle).toHaveBeenCalledTimes(0)
   })
 
@@ -344,9 +345,9 @@ describe('run', () => {
 
   test('build & deploy with --no-actions', async () => {
     command.getAppExtConfigs.mockResolvedValueOnce(createAppConfig(command.appConfig))
-
-    command.argv = ['--no-actions']
+    command.argv = ['--no-actions', '--force-build']
     mockFS.existsSync.mockReturnValue(true)
+
     await command.run()
     expect(command.error).toHaveBeenCalledTimes(0)
     expect(mockWebLib.bundle).toHaveBeenCalledTimes(1)

test/commands/app/deploy.test.js

@@ -194,7 +194,7 @@ test('flags', async () => {
 
   expect(typeof TheCommand.flags['force-build']).toBe('object')
   expect(typeof TheCommand.flags['force-build'].description).toBe('string')
-  expect(TheCommand.flags['force-build'].default).toEqual(true)
+  expect(TheCommand.flags['force-build'].default).toEqual(false)
   expect(TheCommand.flags['force-build'].allowNo).toEqual(true)
 
   expect(typeof TheCommand.flags['content-hash']).toBe('object')
@@ -293,7 +293,10 @@ describe('run', () => {
     expect(mockRuntimeLib.deployActions).toHaveBeenCalledTimes(1)
     expect(mockWebLib.deployWeb).toHaveBeenCalledTimes(1)
     expect(command.buildOneExt).toHaveBeenCalledTimes(1)
-    expect(command.buildOneExt).toHaveBeenCalledWith('application', appConfig.application, expect.objectContaining({ 'force-build': true, verbose: true }), expect.anything())
+    expect(command.buildOneExt).toHaveBeenCalledWith('application',
+      appConfig.application,
+      expect.objectContaining({ 'force-build': false, verbose: true }),
+      expect.anything())
   })
 
   test('build & deploy --no-web-assets', async () => {
@@ -306,7 +309,10 @@ describe('run', () => {
     expect(mockRuntimeLib.deployActions).toHaveBeenCalledTimes(1)
     expect(mockWebLib.deployWeb).toHaveBeenCalledTimes(0)
     expect(command.buildOneExt).toHaveBeenCalledTimes(1)
-    expect(command.buildOneExt).toHaveBeenCalledWith('application', appConfig.application, expect.objectContaining({ 'force-build': true, 'web-assets': false }), expect.anything())
+    expect(command.buildOneExt).toHaveBeenCalledWith('application',
+      appConfig.application,
+      expect.objectContaining({ 'force-build': false, 'web-assets': false }),
+      expect.anything())
   })
 
   test('build & deploy only one action using --action (workspace: Production)', async () => {
@@ -347,9 +353,13 @@ describe('run', () => {
     expect(command.buildOneExt).toHaveBeenCalledTimes(1)
     expect(mockLibConsoleCLI.getApplicationExtensions).toHaveBeenCalledTimes(0)
 
-    expect(command.buildOneExt).toHaveBeenCalledWith('application', appConfig.application, expect.objectContaining({ 'force-build': true, 'web-assets': false, action: ['a', 'b', 'c'] }), expect.anything())
+    expect(command.buildOneExt).toHaveBeenCalledWith('application',
+      appConfig.application,
+      expect.objectContaining({ 'force-build': false, 'web-assets': false, action: ['a', 'b', 'c'] }),
+      expect.anything())
     expect(mockRuntimeLib.deployActions).toHaveBeenCalledWith(appConfig.application, {
-      filterEntities: { actions: ['a', 'b', 'c'] }
+      filterEntities: { actions: ['a', 'b', 'c'] },
+      useForce: false
     },
     expect.any(Function))
   })
@@ -365,9 +375,13 @@ describe('run', () => {
     expect(mockWebLib.deployWeb).toHaveBeenCalledTimes(0)
     expect(command.buildOneExt).toHaveBeenCalledTimes(1)
 
-    expect(command.buildOneExt).toHaveBeenCalledWith('application', appConfig.application, expect.objectContaining({ 'force-build': true, 'web-assets': false, action: ['c'] }), expect.anything())
+    expect(command.buildOneExt).toHaveBeenCalledWith('application',
+      appConfig.application,
+      expect.objectContaining({ 'force-build': false, 'web-assets': false, action: ['c'] }),
+      expect.anything())
     expect(mockRuntimeLib.deployActions).toHaveBeenCalledWith(appConfig.application, {
-      filterEntities: { actions: ['c'] }
+      filterEntities: { actions: ['c'] },
+      useForce: false
     },
     expect.any(Function))
   })
@@ -384,7 +398,10 @@ describe('run', () => {
     expect(mockRuntimeLib.deployActions).toHaveBeenCalledTimes(0)
     expect(mockWebLib.deployWeb).toHaveBeenCalledTimes(0)
     expect(command.buildOneExt).toHaveBeenCalledTimes(1)
-    expect(command.buildOneExt).toHaveBeenCalledWith('application', appConfig.application, expect.objectContaining({ 'force-build': true, 'web-assets': false }), expect.anything())
+    expect(command.buildOneExt).toHaveBeenCalledWith('application',
+      appConfig.application,
+      expect.objectContaining({ 'force-build': false, 'web-assets': false }),
+      expect.anything())
   })
 
   test('build & deploy actions with no actions folder but with a manifest', async () => {
@@ -408,7 +425,10 @@ describe('run', () => {
     expect(mockRuntimeLib.deployActions).toHaveBeenCalledTimes(0)
     expect(mockWebLib.deployWeb).toHaveBeenCalledTimes(1)
     expect(command.buildOneExt).toHaveBeenCalledTimes(1)
-    expect(command.buildOneExt).toHaveBeenCalledWith('application', appConfig.application, expect.objectContaining({ 'force-build': true, actions: false }), expect.anything())
+    expect(command.buildOneExt).toHaveBeenCalledWith('application',
+      appConfig.application,
+      expect.objectContaining({ 'force-build': false, actions: false }),
+      expect.anything())
   })
 
   test('build & deploy with --no-actions with no static folder', async () => {
@@ -423,7 +443,10 @@ describe('run', () => {
     expect(mockRuntimeLib.deployActions).toHaveBeenCalledTimes(0)
     expect(mockWebLib.deployWeb).toHaveBeenCalledTimes(0)
     expect(command.buildOneExt).toHaveBeenCalledTimes(1)
-    expect(command.buildOneExt).toHaveBeenCalledWith('application', appConfig.application, expect.objectContaining({ 'force-build': true, actions: false }), expect.anything())
+    expect(command.buildOneExt).toHaveBeenCalledWith('application',
+      appConfig.application,
+      expect.objectContaining({ 'force-build': false, actions: false }),
+      expect.anything())
   })
 
   test('build & deploy with no manifest.yml', async () => {
@@ -437,7 +460,10 @@ describe('run', () => {
     expect(mockRuntimeLib.deployActions).toHaveBeenCalledTimes(0)
     expect(mockWebLib.deployWeb).toHaveBeenCalledTimes(1)
     expect(command.buildOneExt).toHaveBeenCalledTimes(1)
-    expect(command.buildOneExt).toHaveBeenCalledWith('application', appConfig.application, expect.objectContaining({ 'force-build': true }), expect.anything())
+    expect(command.buildOneExt).toHaveBeenCalledWith('application',
+      appConfig.application,
+      expect.objectContaining({ 'force-build': false }),
+      expect.anything())
   })
 
   test('--no-build', async () => {
@@ -900,6 +926,7 @@ describe('run', () => {
     expect(mockLibConsoleCLI.getApplicationExtensions).toHaveBeenCalledTimes(1)
     expect(mockWebLib.deployWeb).toHaveBeenCalledTimes(1)
     expect(mockRuntimeLib.deployActions).toHaveBeenCalledTimes(1)
+    expect(mockRuntimeLib.deployActions).toHaveBeenCalledWith(expect.any(Object), expect.objectContaining({ useForce: true }), expect.any(Function))
     expect(mockLibConsoleCLI.updateExtensionPoints).toHaveBeenCalledTimes(0)
     expect(mockLibConsoleCLI.updateExtensionPointsWithoutOverwrites).toHaveBeenCalledTimes(0)
   })
@@ -928,6 +955,7 @@ describe('run', () => {
     expect(mockLibConsoleCLI.getApplicationExtensions).toHaveBeenCalledTimes(1)
     expect(mockWebLib.deployWeb).toHaveBeenCalledTimes(1)
     expect(mockRuntimeLib.deployActions).toHaveBeenCalledTimes(1)
+    expect(mockRuntimeLib.deployActions).toHaveBeenCalledWith(expect.any(Object), expect.objectContaining({ useForce: true }), expect.any(Function))
     expect(mockLibConsoleCLI.updateExtensionPoints).toHaveBeenCalledTimes(0)
     expect(mockLibConsoleCLI.updateExtensionPointsWithoutOverwrites).toHaveBeenCalledTimes(0)
   })

test/commands/lib/run-dev.test.js

@@ -409,11 +409,13 @@ test('isLocal false, build-static hook set, serve-static hook not set)', async (
   expect(actionsWatcher).toHaveBeenCalled()
 })
 
-test('runDev always force builds', async () => {
+test('runDev does not always force builds', async () => {
   const config = cloneDeep(createAppConfig().application)
 
   await runDev(config, DATA_DIR, { isLocal: false })
 
   expect(buildActions).toHaveBeenCalled()
-  expect(buildActions).toHaveBeenCalledWith(expect.any(Object) /* config */, null /* filterActions */, true /* forceBuild */)
+  expect(buildActions).toHaveBeenCalledWith(expect.any(Object) /* config */,
+    null /* filterActions */,
+    false /* forceBuild */)
 })