feat(error-handling): handle errors thrown by callback argument

index.d.ts

@@ -9,7 +9,7 @@ export interface TransformResult {
 }
 export function localImport(callback: (path: string) => string): Plugin
 export class Plugin {
-  /** Public plugin properties https://rollupjs.org/guide/en/#properties */
+  /** Name of the Rollup plugin https://rollupjs.org/guide/en/#name */
   name: string
   /** Build hook: https://rollupjs.org/guide/en/#transform */
   transform(sourceCode: string, filename: string): TransformResult

src/lib.rs

@@ -1,4 +1,4 @@
-use napi::{Env, Error, JsFunction, JsString, Ref};
+use napi::{Env, Error, JsError, JsFunction, JsString, JsUnknown, Ref};
 use napi_derive::napi;
 
 mod parser;
@@ -32,18 +32,24 @@ impl Plugin {
         let transform_paths = Box::new(move |path: String| {
             let args: [JsString; 1] = [env.create_string(&path).unwrap()];
 
-            let modified = callback
-                .call(None, &args)
-                .unwrap()
-                .coerce_to_string()
-                .unwrap()
-                .into_utf8()
-                .unwrap();
+            match callback.call(None, &args) {
+                Ok(new_path) => Ok(to_string(new_path)),
+                Err(e) => {
+                    // Capture the error thrown from JS side
+                    let error_from_callback = to_string(JsError::from(e).into_unknown(env));
 
-            return modified.as_str().unwrap().to_string();
+                    return Err(format!(
+                        "Callback threw error {:?} when called with {:?}",
+                        &error_from_callback, &path
+                    ));
+                }
+            }
         });
 
-        parser::parse(&source_code, &filename, transform_paths)
+        match parser::parse(&source_code, &filename, transform_paths) {
+            Ok(result) => Ok(result),
+            Err(error) => Err(Error::new(napi::Status::GenericFailure, error)),
+        }
     }
 
     /// Build hook: https://rollupjs.org/guide/en/#buildend
@@ -77,3 +83,14 @@ pub fn local_import(env: Env, callback: JsFunction) -> Result<Plugin, Error> {
         callback_reference,
     })
 }
+
+fn to_string(js_unknown: JsUnknown) -> String {
+    js_unknown
+        .coerce_to_string()
+        .unwrap()
+        .into_utf8()
+        .unwrap()
+        .as_str()
+        .unwrap()
+        .to_string()
+}

src/parser.rs

@@ -20,19 +20,22 @@ use swc_core::{
 
 use crate::TransformResult;
 
-type Callback = Box<dyn Fn(String) -> String>;
+type Callback = Box<dyn Fn(String) -> Result<String, String>>;
 
 struct Visitor {
     callback: Callback,
+    errors: Vec<String>,
 }
 
 impl VisitMut for Visitor {
     fn visit_mut_export_all(&mut self, node: &mut ExportAll) {
         let path = node.src.value.to_string();
 
         if is_local_import(&path) {
-            let new_path = (&self.callback)(path);
-            node.src.value = new_path.into()
+            match (&self.callback)(path) {
+                Ok(new_path) => node.src.value = new_path.into(),
+                Err(error) => self.errors.push(error),
+            }
         }
     }
 
@@ -46,17 +49,21 @@ impl VisitMut for Visitor {
         };
 
         if is_local_import(&path) {
-            let new_path = (&self.callback)(path);
-            node.src.as_mut().unwrap().value = new_path.into()
+            match (&self.callback)(path) {
+                Ok(new_path) => node.src.as_mut().unwrap().value = new_path.into(),
+                Err(error) => self.errors.push(error),
+            };
         }
     }
 
     fn visit_mut_import_decl(&mut self, node: &mut ImportDecl) {
         let path = node.src.value.to_string();
 
         if is_local_import(&path) {
-            let new_path = (&self.callback)(path);
-            node.src.value = new_path.into()
+            match (&self.callback)(path) {
+                Ok(new_path) => node.src.value = new_path.into(),
+                Err(error) => self.errors.push(error),
+            };
         }
     }
 }
@@ -65,11 +72,11 @@ fn is_local_import(path: &str) -> bool {
     return path.starts_with("./") || path.starts_with("../");
 }
 
-fn visitor(callback: Callback) -> impl Fold {
-    as_folder(Visitor { callback })
+fn as_visitor(visitor: &mut Visitor) -> impl Fold + '_ {
+    as_folder(visitor)
 }
 
-pub fn parse(code: &str, filename: &str, callback: Callback) -> napi::Result<TransformResult> {
+pub fn parse(code: &str, filename: &str, callback: Callback) -> Result<TransformResult, String> {
     let source_map: Lrc<SourceMap> = Default::default();
     let source_file =
         source_map.new_source_file(FileName::Custom(filename.to_string()), code.to_string());
@@ -78,6 +85,11 @@ pub fn parse(code: &str, filename: &str, callback: Callback) -> napi::Result<Tra
 
     let compiler = Compiler::new(source_map);
 
+    let mut visitor = Visitor {
+        callback,
+        errors: vec![],
+    };
+
     let transformed = compiler.process_js_with_custom_pass(
         source_file,
         None,
@@ -87,10 +99,18 @@ pub fn parse(code: &str, filename: &str, callback: Callback) -> napi::Result<Tra
             source_maps: Some(SourceMapsConfig::Bool(true)),
             ..Default::default()
         },
-        |_, _| visitor(callback),
+        |_, _| as_visitor(&mut visitor),
         |_, _| noop(),
     );
 
+    if visitor.errors.len() > 0 {
+        return Err(format!(
+            "Run into {:?} error(s): [{:?}].",
+            visitor.errors.len(),
+            visitor.errors.join(",")
+        ));
+    }
+
     handler.abort_if_errors();
 
     let TransformOutput {
@@ -99,21 +119,13 @@ pub fn parse(code: &str, filename: &str, callback: Callback) -> napi::Result<Tra
     } = match transformed {
         Ok(result) => result,
         Err(error) => {
-            return Err(napi::Error::new(
-                napi::Status::GenericFailure,
-                error.to_string(),
-            ));
+            return Err(error.to_string());
         }
     };
 
     match option_map {
         Some(map) => Ok(TransformResult { code, map }),
-        None => {
-            return Err(napi::Error::new(
-                napi::Status::GenericFailure,
-                String::from("Failed to generate sourcemaps."),
-            ));
-        }
+        None => Err("Failed to generate sourcemaps.".to_string()),
     }
 }
 
@@ -126,7 +138,7 @@ mod tests {
             let mut new_path: String = path.to_string();
             new_path.push_str(".js");
 
-            new_path
+            Ok(new_path)
         })
     }
 
@@ -137,7 +149,7 @@ mod tests {
             "some-file.js",
             Box::new(|path: String| {
                 assert_eq!(path, "./local-file");
-                path
+                Ok(path)
             }),
         )
         .unwrap();

test/error-handling.test.js

@@ -31,13 +31,33 @@ test("throws useful error message when callback is not function", () => {
   );
 });
 
-test.todo("throws useful error message when callback throws", () => {
+test("throws useful error message when callback throws", () => {
   const plugin = localImport(() => {
-    throw new Error("Error from callback");
+    throw new Error("Throwing some error from callback");
   });
   cleanups.push(plugin.buildEnd);
 
   expect(() =>
-    plugin.transform('import fs from "./some-file";')
-  ).toThrowErrorMatchingInlineSnapshot();
+    plugin.transform('import A from "./some-file";', "file.js")
+  ).toThrowErrorMatchingInlineSnapshot('"Run into 1 error(s): [\\"Callback threw error \\\\\\"Error: Throwing some error from callback\\\\\\" when called with \\\\\\"./some-file\\\\\\"\\"]."');
+});
+
+test("includes all errors thrown by callback in error message", () => {
+  const plugin = localImport(() => {
+    throw new Error("Throwing some error from callback");
+  });
+  cleanups.push(plugin.buildEnd);
+
+  expect(() =>
+    plugin.transform(
+      `
+    export * from "./local-file-1";
+    export { a } from "./local-file-2";
+    import b from "./local-file-3";
+    import { c } from "./local-file-4";
+    import "./local-file-5";
+    `,
+      "file.js"
+    )
+  ).toThrowErrorMatchingInlineSnapshot('"Run into 5 error(s): [\\"Callback threw error \\\\\\"Error: Throwing some error from callback\\\\\\" when called with \\\\\\"./local-file-1\\\\\\",Callback threw error \\\\\\"Error: Throwing some error from callback\\\\\\" when called with \\\\\\"./local-file-2\\\\\\",Callback threw error \\\\\\"Error: Throwing some error from callback\\\\\\" when called with \\\\\\"./local-file-3\\\\\\",Callback threw error \\\\\\"Error: Throwing some error from callback\\\\\\" when called with \\\\\\"./local-file-4\\\\\\",Callback threw error \\\\\\"Error: Throwing some error from callback\\\\\\" when called with \\\\\\"./local-file-5\\\\\\"\\"]."');
 });