Macro errors -> v2 (#9501)

* Improve macro error handling

* flow

crates/node-bindings/src/transformer.rs

@@ -18,12 +18,12 @@ mod native_only {
     threadsafe_function::{ThreadSafeCallContext, ThreadsafeFunctionCallMode},
     JsBoolean, JsFunction, JsNumber, JsString, ValueType,
   };
-  use parcel_js_swc_core::{JsValue, SourceLocation};
+  use parcel_js_swc_core::{JsValue, MacroError, SourceLocation};
   use std::sync::Arc;
 
   // Allocate a single channel per thread to communicate with the JS thread.
   thread_local! {
-    static CHANNEL: (Sender<Result<JsValue, String>>, Receiver<Result<JsValue, String>>) = crossbeam_channel::unbounded();
+    static CHANNEL: (Sender<Result<JsValue, MacroError>>, Receiver<Result<JsValue, MacroError>>) = crossbeam_channel::unbounded();
   }
 
   struct CallMacroMessage {
@@ -33,6 +33,12 @@ mod native_only {
     loc: SourceLocation,
   }
 
+  #[napi(object)]
+  struct JsMacroError {
+    pub kind: u32,
+    pub message: String,
+  }
+
   #[napi]
   pub fn transform_async(opts: JsObject, env: Env) -> napi::Result<JsObject> {
     let call_macro_tsfn = if opts.has_named_property("callMacro")? {
@@ -203,7 +209,7 @@ mod native_only {
   fn await_promise(
     env: Env,
     result: JsUnknown,
-    tx: Sender<Result<JsValue, String>>,
+    tx: Sender<Result<JsValue, MacroError>>,
   ) -> napi::Result<()> {
     // If the result is a promise, wait for it to resolve, and send the result to the channel.
     // Otherwise, send the result immediately.
@@ -217,12 +223,13 @@ mod native_only {
         ctx.env.get_undefined()
       })?;
       let eb = env.create_function_from_closure("error_callback", move |ctx| {
-        let res = ctx.get::<JsUnknown>(0)?;
-        let message = match napi_to_js_value(res, env)? {
-          JsValue::String(s) => s,
-          _ => "Unknown error".into(),
+        let res = ctx.get::<JsMacroError>(0)?;
+        let err = match res.kind {
+          1 => MacroError::LoadError(res.message),
+          2 => MacroError::ExecutionError(res.message),
+          _ => MacroError::LoadError("Invalid error kind".into()),
         };
-        tx2.send(Err(message)).expect("send failure");
+        tx2.send(Err(err)).expect("send failure");
         ctx.env.get_undefined()
       })?;
       then.call(Some(&result), &[cb, eb])?;

packages/core/core/src/Transformation.js

@@ -264,24 +264,24 @@ export default class Transformation {
     initialAsset: UncommittedAsset,
   ): Promise<Array<UncommittedAsset>> {
     let initialType = initialAsset.value.type;
-    let assets: Array<UncommittedAsset> = await this.runPipeline(
-      pipeline,
-      initialAsset,
-    );
-
-    // Add dev dep requests for each transformer
-    for (let transformer of pipeline.transformers) {
-      await this.addDevDependency({
-        specifier: transformer.name,
-        resolveFrom: transformer.resolveFrom,
-        range: transformer.range,
-      });
-    }
+    let assets: Array<UncommittedAsset>;
+    try {
+      assets = await this.runPipeline(pipeline, initialAsset);
+    } finally {
+      // Add dev dep requests for each transformer
+      for (let transformer of pipeline.transformers) {
+        await this.addDevDependency({
+          specifier: transformer.name,
+          resolveFrom: transformer.resolveFrom,
+          range: transformer.range,
+        });
+      }
 
-    // Add dev dep requests for dependencies of transformer plugins
-    // (via proxied packageManager.require calls).
-    for (let devDep of this.pluginDevDeps) {
-      await this.addDevDependency(devDep);
+      // Add dev dep requests for dependencies of transformer plugins
+      // (via proxied packageManager.require calls).
+      for (let devDep of this.pluginDevDeps) {
+        await this.addDevDependency(devDep);
+      }
     }
 
     let finalAssets: Array<UncommittedAsset> = [];
@@ -324,13 +324,17 @@ export default class Transformation {
     }
 
     // Ensure that the package manager has an entry for this resolution.
-    await this.options.packageManager.resolve(
-      specifier,
-      fromProjectPath(this.options.projectRoot, opts.resolveFrom),
-      {
-        range,
-      },
-    );
+    try {
+      await this.options.packageManager.resolve(
+        specifier,
+        fromProjectPath(this.options.projectRoot, opts.resolveFrom),
+        {
+          range,
+        },
+      );
+    } catch (err) {
+      // ignore
+    }
 
     let devDepRequest = await createDevDependency(
       opts,

packages/core/core/src/requests/DevDepRequest.js

@@ -49,7 +49,12 @@ export async function createDevDependency(
   let resolveFromAbsolute = fromProjectPath(options.projectRoot, resolveFrom);
 
   // Ensure that the package manager has an entry for this resolution.
-  await options.packageManager.resolve(specifier, resolveFromAbsolute);
+  try {
+    await options.packageManager.resolve(specifier, resolveFromAbsolute);
+  } catch (err) {
+    // ignore
+  }
+
   let invalidations = options.packageManager.getInvalidations(
     specifier,
     resolveFromAbsolute,

packages/core/integration-tests/test/macros.js

@@ -1,7 +1,15 @@
 // @flow strict-local
 import assert from 'assert';
+import invariant from 'assert';
 import path from 'path';
-import {bundle, run, overlayFS, fsFixture} from '@parcel/test-utils';
+import {
+  bundle,
+  bundler,
+  run,
+  overlayFS,
+  fsFixture,
+  getNextBuild,
+} from '@parcel/test-utils';
 
 describe('macros', function () {
   let count = 0;
@@ -368,7 +376,7 @@ describe('macros', function () {
     } catch (err) {
       assert.deepEqual(err.diagnostics, [
         {
-          message: `Error evaluating macro: Could not resolve module "./macro.js" from "${path.join(
+          message: `Error loading macro: Could not resolve module "./macro.js" from "${path.join(
             dir,
             'index.js',
           )}"`,
@@ -380,12 +388,12 @@ describe('macros', function () {
                 {
                   message: undefined,
                   start: {
-                    line: 2,
-                    column: 10,
+                    line: 1,
+                    column: 1,
                   },
                   end: {
-                    line: 2,
-                    column: 19,
+                    line: 1,
+                    column: 57,
                   },
                 },
               ],
@@ -561,6 +569,75 @@ describe('macros', function () {
     assert.notEqual(match[1], match2[1]);
   });
 
+  it('should only error once if a macro errors during loading', async function () {
+    await fsFixture(overlayFS, dir)`
+      index.js:
+        import { test } from "./macro.js" with { type: "macro" };
+        output = test(1, 2);
+        output2 = test(1, 3);
+
+      macro.js:
+        export function test() {
+          return Date.now(
+        }
+    `;
+
+    try {
+      await bundle(path.join(dir, '/index.js'), {
+        inputFS: overlayFS,
+        mode: 'production',
+      });
+    } catch (err) {
+      assert.equal(err.diagnostics.length, 1);
+    }
+  });
+
+  it('should rebuild in watch mode after fixing an error', async function () {
+    await fsFixture(overlayFS, dir)`
+      index.js:
+        import { test } from "./macro.ts" with { type: "macro" };
+        output = test('test.txt');
+
+      macro.ts:
+        export function test() {
+          return Date.now(
+        }
+    `;
+
+    let b = await bundler(path.join(dir, '/index.js'), {
+      inputFS: overlayFS,
+      mode: 'production',
+      shouldDisableCache: false,
+    });
+
+    let subscription;
+    try {
+      subscription = await b.watch();
+      let buildEvent = await getNextBuild(b);
+      assert.equal(buildEvent.type, 'buildFailure');
+
+      await fsFixture(overlayFS, dir)`
+        macro.ts:
+          export function test() {
+            return Date.now();
+          }
+      `;
+
+      buildEvent = await getNextBuild(b);
+      assert.equal(buildEvent.type, 'buildSuccess');
+      invariant(buildEvent.type === 'buildSuccess'); // flow
+
+      let res = await overlayFS.readFile(
+        buildEvent.bundleGraph.getBundles()[0].filePath,
+        'utf8',
+      );
+      let match = res.match(/output=(\d+)/);
+      assert(match);
+    } finally {
+      await subscription?.unsubscribe();
+    }
+  });
+
   it('should support evaluating constants', async function () {
     await fsFixture(overlayFS, dir)`
       index.js:

packages/transformers/js/core/src/lib.rs

@@ -53,8 +53,8 @@ use node_replacer::NodeReplacer;
 use typeof_replacer::*;
 use utils::{error_buffer_to_diagnostics, Diagnostic, DiagnosticSeverity, ErrorBuffer, SourceType};
 
-pub use crate::macros::JsValue;
 use crate::macros::Macros;
+pub use crate::macros::{JsValue, MacroError};
 pub use utils::SourceLocation;
 
 type SourceMapBuffer = Vec<(swc_core::common::BytePos, swc_core::common::LineCol)>;