fix: cli no longer crashes on syntax errors (#239)

feat: print syntax error location

CHANGES.md

@@ -1,5 +1,12 @@
 # History of Changes
 
+## Unreleased
+
+### Bug Fixes
+
+- CLI no longer crashes on syntax errors and reports errors with location
+- Dynamic imports no longer blows up build
+
 This is a history of changes to [Remix](https://remix.run).
 
 ## 0.17.3 - Wed May 19 2021

packages/remix-dev/cli/commands.ts

@@ -55,10 +55,6 @@ export async function watch(
   signalExit(
     await compiler.watch(config, {
       mode,
-      // TODO: esbuild compiler just blows up on syntax errors in the app
-      // onError(errorMessage) {
-      //   console.error(errorMessage);
-      // },
       onRebuildStart() {
         start = Date.now();
         onRebuildStart && onRebuildStart();

packages/remix-dev/compiler.ts

@@ -30,13 +30,31 @@ function defaultWarningHandler(message: string, key: string) {
   warnOnce(false, message, key);
 }
 
-function defaultErrorHandler(message: string) {
-  console.error(message);
+function defaultBuildFailureHandler(failure: Error | esbuild.BuildFailure) {
+  if ("warnings" in failure || "errors" in failure) {
+    if (failure.warnings) {
+      let messages = esbuild.formatMessagesSync(failure.warnings, {
+        kind: "warning",
+        color: true
+      });
+      console.warn(...messages);
+    }
+
+    if (failure.errors) {
+      let messages = esbuild.formatMessagesSync(failure.errors, {
+        kind: "error",
+        color: true
+      });
+      console.error(...messages);
+    }
+  }
+
+  console.error(failure?.message || "An unknown build error occured");
 }
 
 interface BuildOptions extends Partial<BuildConfig> {
   onWarning?(message: string, key: string): void;
-  onError?(message: string): void;
+  onBuildFailure?(failure: Error | esbuild.BuildFailure): void;
 }
 
 export async function build(
@@ -45,10 +63,15 @@ export async function build(
     mode = BuildMode.Production,
     target = BuildTarget.Node14,
     onWarning = defaultWarningHandler,
-    onError = defaultErrorHandler
+    onBuildFailure = defaultBuildFailureHandler
   }: BuildOptions = {}
 ): Promise<void> {
-  await buildEverything(config, { mode, target, onWarning, onError });
+  await buildEverything(config, {
+    mode,
+    target,
+    onWarning,
+    onBuildFailure
+  });
 }
 
 interface WatchOptions extends BuildOptions {
@@ -65,21 +88,27 @@ export async function watch(
     mode = BuildMode.Development,
     target = BuildTarget.Node14,
     onWarning = defaultWarningHandler,
-    onError = defaultErrorHandler,
+    onBuildFailure = defaultBuildFailureHandler,
     onRebuildStart,
     onRebuildFinish,
     onFileCreated,
     onFileChanged,
     onFileDeleted
   }: WatchOptions = {}
 ): Promise<() => void> {
-  let options = { mode, target, onWarning, onError, incremental: true };
+  let options = {
+    mode,
+    target,
+    onBuildFailure,
+    onWarning,
+    incremental: true
+  };
   let [browserBuild, serverBuild] = await buildEverything(config, options);
 
   async function disposeBuilders() {
     await Promise.all([
-      browserBuild.rebuild?.dispose(),
-      serverBuild.rebuild?.dispose()
+      browserBuild?.rebuild?.dispose(),
+      serverBuild?.rebuild?.dispose()
     ]);
   }
 
@@ -95,12 +124,29 @@ export async function watch(
 
   let rebuildEverything = debounce(async () => {
     if (onRebuildStart) onRebuildStart();
+
+    if (!browserBuild || !serverBuild) {
+      await disposeBuilders();
+
+      try {
+        [browserBuild, serverBuild] = await buildEverything(config, options);
+        if (onRebuildFinish) onRebuildFinish();
+      } catch (err) {
+        onBuildFailure(err);
+      }
+      return;
+    }
+
     await Promise.all([
+      // If we get here and can't call rebuild something went wrong and we
+      // should probably blow as it's not really recoverable.
       browserBuild.rebuild!().then(build =>
         generateManifests(config, build.metafile!)
       ),
       serverBuild.rebuild!()
-    ]);
+    ]).catch(err => {
+      onBuildFailure(err);
+    });
     if (onRebuildFinish) onRebuildFinish();
   }, 100);
 
@@ -163,7 +209,7 @@ function isEntryPoint(config: RemixConfig, file: string) {
 async function buildEverything(
   config: RemixConfig,
   options: Required<BuildOptions> & { incremental?: boolean }
-): Promise<esbuild.BuildResult[]> {
+): Promise<(esbuild.BuildResult | undefined)[]> {
   // TODO:
   // When building for node, we build both the browser and server builds in
   // parallel and emit the asset manifest as a separate file in the output
@@ -181,7 +227,10 @@ async function buildEverything(
       return build;
     }),
     serverBuildPromise
-  ]);
+  ]).catch(err => {
+    options.onBuildFailure(err);
+    return [undefined, undefined];
+  });
 }
 
 async function createBrowserBuild(
@@ -216,6 +265,7 @@ async function createBrowserBuild(
     inject: [reactShim],
     loader: loaders,
     bundle: true,
+    logLevel: "silent",
     splitting: true,
     metafile: true,
     incremental: options.incremental,
@@ -252,6 +302,7 @@ async function createServerBuild(
     inject: [reactShim],
     loader: loaders,
     bundle: true,
+    logLevel: "silent",
     incremental: options.incremental,
     // The server build needs to know how to generate asset URLs for imports
     // of CSS and other files.
@@ -406,9 +457,21 @@ function browserRouteModulesPlugin(
           let route = routesByFile.get(file);
           invariant(route, `Cannot get route by path: ${args.path}`);
 
-          let exports = (
-            await getRouteModuleExportsCached(config, route.id)
-          ).filter(ex => !!browserSafeRouteExports[ex]);
+          let exports;
+          try {
+            exports = (
+              await getRouteModuleExportsCached(config, route.id)
+            ).filter(ex => !!browserSafeRouteExports[ex]);
+          } catch (error) {
+            return {
+              errors: [
+                {
+                  text: error.message,
+                  pluginName: "browser-route-module"
+                }
+              ]
+            };
+          }
           let spec = exports.length > 0 ? `{ ${exports.join(", ")} }` : "*";
           let contents = `export ${spec} from ${JSON.stringify(file)};`;
 

packages/remix-dev/compiler/assets.ts

@@ -78,7 +78,8 @@ export async function createAssetsManifest(
         module: resolveUrl(key),
         imports: resolveImports(output.imports)
       };
-    } else {
+      // Only parse routes otherwise dynamic imports can fall into here and fail the build
+    } else if (output.entryPoint.startsWith("browser-route-module:")) {
       let route = routesByFile.get(entryPointFile);
       invariant(route, `Cannot get route for entry point ${output.entryPoint}`);
       let sourceExports = await getRouteModuleExportsCached(config, route.id);

packages/remix-dev/compiler/routes.ts

@@ -46,7 +46,8 @@ export async function getRouteModuleExports(
     platform: "neutral",
     format: "esm",
     metafile: true,
-    write: false
+    write: false,
+    logLevel: "silent"
   });
   let metafile = result.metafile!;