Merge pull request #901 from spotify/rugvip/cleanup

build setup cleanup / PR feedback

packages/cli/config/jest.js

@@ -37,6 +37,7 @@ async function getConfig() {
 
     // To avoid having to build all deps inside the monorepo before running tests,
     // we point directory to src/ where applicable.
+    // For example, @backstage/core = <repo-root>/packages/core/src/index.ts is added to moduleNameMapper
     for (const pkg of packages) {
       const mainSrc = pkg.get('main:src');
       if (mainSrc) {

packages/cli/src/commands/app/build.ts

@@ -22,7 +22,4 @@ export default async (cmd: Command) => {
     entry: 'src/index',
     statsJsonEnabled: cmd.stats,
   });
-
-  // Wait for interrupt signal
-  await new Promise(() => {});
 };

packages/cli/src/commands/app/serve.ts

@@ -18,11 +18,10 @@ import { serveBundle } from '../../lib/bundler';
 import { Command } from 'commander';
 
 export default async (cmd: Command) => {
-  await serveBundle({
+  const waitForExit = await serveBundle({
     entry: 'src/index',
     checksEnabled: cmd.check,
   });
 
-  // Wait for interrupt signal
-  await new Promise(() => {});
+  await waitForExit();
 };

packages/cli/src/commands/plugin/serve.ts

@@ -18,11 +18,10 @@ import { serveBundle } from '../../lib/bundler';
 import { Command } from 'commander';
 
 export default async (cmd: Command) => {
-  await serveBundle({
+  const waitForExit = await serveBundle({
     entry: 'dev/index',
     checksEnabled: cmd.check,
   });
 
-  // Wait for interrupt signal
-  await new Promise(() => {});
+  await waitForExit();
 };

packages/cli/src/lib/bundler/server.ts

@@ -29,7 +29,7 @@ export async function serveBundle(options: ServeOptions) {
 
   const port = await choosePort(host, defaultPort);
   if (!port) {
-    return;
+    throw new Error(`Invalid or no port set: '${port}'`);
   }
 
   const protocol = yn(process.env.HTTPS, { default: false }) ? 'https' : 'http';
@@ -57,15 +57,23 @@ export async function serveBundle(options: ServeOptions) {
         return;
       }
 
-      for (const signal of ['SIGINT', 'SIGTERM'] as const) {
-        process.on(signal, () => {
-          server.close();
-          process.exit();
-        });
-      }
-
       openBrowser(urls.localUrlForBrowser);
       resolve();
     });
   });
+
+  const waitForExit = async () => {
+    for (const signal of ['SIGINT', 'SIGTERM'] as const) {
+      process.on(signal, () => {
+        server.close();
+        // exit instead of resolve. The process is shutting down and resolving a promise here logs an error
+        process.exit();
+      });
+    }
+
+    // Block indefinitely and wait for the interrupt signal
+    return new Promise(() => {});
+  };
+
+  return waitForExit;
 }

packages/cli/src/lib/packager/packager.ts

@@ -24,7 +24,6 @@ function formatErrorMessage(error: any) {
   let msg = '';
 
   if (error.code === 'PLUGIN_ERROR') {
-    // typescript2 plugin has a complete message with all codeframes
     if (error.plugin === 'esbuild') {
       msg += `${error.message}\n\n`;
       for (const { text, location } of error.errors) {

packages/cli/src/lib/tasks.ts

@@ -118,8 +118,11 @@ const PATCH_PACKAGES = [
   'theme',
 ];
 
+// This runs a `yarn install` task, but with special treatment for e2e tests
 export async function installWithLocalDeps(dir: string) {
-  // e2e testing needs special treatment
+  // This makes us install any package inside this repo as a local file dependency.
+  // For example, instead of trying to fetch @backstage/core from npm, we point it
+  // to <repo-root>/packages/core. This makes yarn use a simple file copy to install it instead.
   if (process.env.BACKSTAGE_E2E_CLI_TEST) {
     Task.section('Linking packages locally for e2e tests');
 
@@ -163,6 +166,11 @@ export async function installWithLocalDeps(dir: string) {
     });
   });
 
+  // This takes care of pointing all the installed packages from this repo to
+  // dist instead of the local src.
+  // For example node_modules/@backstage/core/packages.json is rewritten to point
+  // types to dist/index.d.ts and the main:src field is removed.
+  // Without this we get type checking errors in the e2e test
   if (process.env.BACKSTAGE_E2E_CLI_TEST) {
     Task.section('Patchling local dependencies for e2e tests');
 
@@ -177,7 +185,6 @@ export async function installWithLocalDeps(dir: string) {
             name,
             'package.json',
           );
-          console.log('DEBUG: depJsonPath =', depJsonPath);
           const depJson = await fs.readJson(depJsonPath);
 
           // We want dist to be used for e2e tests