Merge pull request #1043 from JoostK/fix-optional-args

fix(napi-derive): an `Option` in front of a required parameter is no …

crates/backend/src/typegen/fn.rs

@@ -1,10 +1,57 @@
 use convert_case::{Case, Casing};
 use quote::ToTokens;
+use std::fmt::{Display, Formatter};
 use syn::Pat;
 
 use super::{ty_to_ts_type, ToTypeDef, TypeDef};
 use crate::{js_doc_from_comments, CallbackArg, FnKind, NapiFn};
 
+struct FnArg {
+  arg: String,
+  ts_type: String,
+  is_optional: bool,
+}
+
+struct FnArgList {
+  args: Vec<FnArg>,
+  last_required: Option<usize>,
+}
+
+impl Display for FnArgList {
+  fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
+    for (i, arg) in self.args.iter().enumerate() {
+      if i != 0 {
+        write!(f, ", ")?;
+      }
+      let is_optional = arg.is_optional
+        && self
+          .last_required
+          .map_or(true, |last_required| i > last_required);
+      if is_optional {
+        write!(f, "{}?: {}", arg.arg, arg.ts_type)?;
+      } else {
+        write!(f, "{}: {}", arg.arg, arg.ts_type)?;
+      }
+    }
+    Ok(())
+  }
+}
+
+impl FromIterator<FnArg> for FnArgList {
+  fn from_iter<T: IntoIterator<Item = FnArg>>(iter: T) -> Self {
+    let args = iter.into_iter().collect::<Vec<_>>();
+    let last_required = args
+      .iter()
+      .enumerate()
+      .rfind(|(_, arg)| !arg.is_optional)
+      .map(|(i, _)| i);
+    FnArgList {
+      args,
+      last_required,
+    }
+  }
+}
+
 impl ToTypeDef for NapiFn {
   fn to_type_def(&self) -> Option<TypeDef> {
     if self.skip_typescript {
@@ -45,15 +92,14 @@ fn gen_callback_type(callback: &CallbackArg) -> String {
       .iter()
       .enumerate()
       .map(|(i, arg)| {
-        let (arg, is_optional) = ty_to_ts_type(arg, false);
+        let (ts_type, is_optional) = ty_to_ts_type(arg, false);
-        if is_optional {
+        FnArg {
-          format!("arg{}?: {}", i, arg)
+          arg: format!("arg{}", i),
-        } else {
+          ts_type,
-          format!("arg{}: {}", i, arg)
+          is_optional,
         }
       })
-      .collect::<Vec<_>>()
+      .collect::<FnArgList>(),
-      .join(", "),
     ret = match &callback.ret {
       Some(ty) => ty_to_ts_type(ty, true).0,
       None => "void".to_owned(),
@@ -63,36 +109,43 @@ fn gen_callback_type(callback: &CallbackArg) -> String {
 
 impl NapiFn {
   fn gen_ts_func_args(&self) -> String {
-    self
+    format!(
-      .args
+      "{}",
-      .iter()
+      self
-      .filter_map(|arg| match arg {
+        .args
-        crate::NapiFnArgKind::PatType(path) => {
+        .iter()
-          if path.ty.to_token_stream().to_string() == "Env" {
+        .filter_map(|arg| match arg {
-            return None;
+          crate::NapiFnArgKind::PatType(path) => {
-          }
+            if path.ty.to_token_stream().to_string() == "Env" {
-          let mut path = path.clone();
+              return None;
-          // remove mutability from PatIdent
+            }
-          if let Pat::Ident(i) = path.pat.as_mut() {
+            let mut path = path.clone();
-            i.mutability = None;
+            // remove mutability from PatIdent
-          }
+            if let Pat::Ident(i) = path.pat.as_mut() {
-          let mut arg = path.pat.to_token_stream().to_string().to_case(Case::Camel);
+              i.mutability = None;
-          let (ts_arg, is_optional) = ty_to_ts_type(&path.ty, false);
+            }
-          arg.push_str(if is_optional { "?: " } else { ": " });
+            let arg = path.pat.to_token_stream().to_string().to_case(Case::Camel);
-          arg.push_str(&ts_arg);
+            let (ts_type, is_optional) = ty_to_ts_type(&path.ty, false);
 
-          Some(arg)
+            Some(FnArg {
-        }
+              arg,
-        crate::NapiFnArgKind::Callback(cb) => {
+              ts_type,
-          let mut arg = cb.pat.to_token_stream().to_string().to_case(Case::Camel);
+              is_optional,
-          arg.push_str(": ");
+            })
-          arg.push_str(&gen_callback_type(cb));
+          }
+          crate::NapiFnArgKind::Callback(cb) => {
+            let arg = cb.pat.to_token_stream().to_string().to_case(Case::Camel);
+            let ts_type = gen_callback_type(cb);
 
-          Some(arg)
+            Some(FnArg {
-        }
+              arg,
-      })
+              ts_type,
-      .collect::<Vec<_>>()
+              is_optional: false,
-      .join(", ")
+            })
+          }
+        })
+        .collect::<FnArgList>()
+    )
   }
 
   fn gen_ts_func_prefix(&self) -> &'static str {

examples/napi/__test__/typegen.spec.ts.md

@@ -32,6 +32,10 @@ Generated by [AVA](https://avajs.dev).
     export function createBigInt(): bigint␊
     export function createBigIntI64(): bigint␊
     export function getCwd(callback: (arg0: string) => void): void␊
+    export function optionEnd(callback: (arg0: string, arg1?: string | undefined | null) => void): void␊
+    export function optionStart(callback: (arg0: string | undefined | null, arg1: string) => void): void␊
+    export function optionStartEnd(callback: (arg0: string | undefined | null, arg1: string, arg2?: string | undefined | null) => void): void␊
+    export function optionOnly(callback: (arg0?: string | undefined | null) => void): void␊
     /** napi = { version = 2, features = ["serde-json"] } */␊
     export function readFile(callback: (arg0: Error | undefined, arg1?: string | undefined | null) => void): void␊
     export function eitherStringOrNumber(input: string | number): number␊
@@ -208,6 +212,12 @@ Generated by [AVA](https://avajs.dev).
       constructor()␊
       get filePath(): number␊
     }␊
+    export class Optional {␊
+      static optionEnd(required: string, optional?: string | undefined | null): string␊
+      static optionStart(optional: string | undefined | null, required: string): string␊
+      static optionStartEnd(optional1: string | undefined | null, required: string, optional2?: string | undefined | null): string␊
+      static optionOnly(optional?: string | undefined | null): string␊
+    }␊
     export class ClassWithFactory {␊
       name: string␊
       static withName(name: string): ClassWithFactory␊

examples/napi/index.d.ts

@@ -22,6 +22,10 @@ export function bigintAdd(a: bigint, b: bigint): bigint
 export function createBigInt(): bigint
 export function createBigIntI64(): bigint
 export function getCwd(callback: (arg0: string) => void): void
+export function optionEnd(callback: (arg0: string, arg1?: string | undefined | null) => void): void
+export function optionStart(callback: (arg0: string | undefined | null, arg1: string) => void): void
+export function optionStartEnd(callback: (arg0: string | undefined | null, arg1: string, arg2?: string | undefined | null) => void): void
+export function optionOnly(callback: (arg0?: string | undefined | null) => void): void
 /** napi = { version = 2, features = ["serde-json"] } */
 export function readFile(callback: (arg0: Error | undefined, arg1?: string | undefined | null) => void): void
 export function eitherStringOrNumber(input: string | number): number
@@ -198,6 +202,12 @@ export class Asset {
   constructor()
   get filePath(): number
 }
+export class Optional {
+  static optionEnd(required: string, optional?: string | undefined | null): string
+  static optionStart(optional: string | undefined | null, required: string): string
+  static optionStartEnd(optional1: string | undefined | null, required: string, optional2?: string | undefined | null): string
+  static optionOnly(optional?: string | undefined | null): string
+}
 export class ClassWithFactory {
   name: string
   static withName(name: string): ClassWithFactory

examples/napi/src/callback.rs

@@ -6,6 +6,26 @@ fn get_cwd<T: Fn(String) -> Result<()>>(callback: T) {
   callback(env::current_dir().unwrap().to_string_lossy().to_string()).unwrap();
 }
 
+#[napi]
+fn option_end<T: Fn(String, Option<String>) -> Result<()>>(callback: T) {
+  callback("Hello".to_string(), None).unwrap();
+}
+
+#[napi]
+fn option_start<T: Fn(Option<String>, String) -> Result<()>>(callback: T) {
+  callback(None, "World".to_string()).unwrap();
+}
+
+#[napi]
+fn option_start_end<T: Fn(Option<String>, String, Option<String>) -> Result<()>>(callback: T) {
+  callback(None, "World".to_string(), None).unwrap();
+}
+
+#[napi]
+fn option_only<T: Fn(Option<String>) -> Result<()>>(callback: T) {
+  callback(None).unwrap();
+}
+
 /// napi = { version = 2, features = ["serde-json"] }
 #[napi]
 fn read_file<T: Fn(Result<()>, Option<String>) -> Result<()>>(callback: T) {

examples/napi/src/class.rs

@@ -237,3 +237,47 @@ impl JsAsset {
     return 1;
   }
 }
+
+#[napi]
+pub struct Optional {}
+
+#[napi]
+impl Optional {
+  #[napi]
+  pub fn option_end(required: String, optional: Option<String>) -> String {
+    match optional {
+      None => required,
+      Some(optional) => format!("{} {}", required, optional),
+    }
+  }
+
+  #[napi]
+  pub fn option_start(optional: Option<String>, required: String) -> String {
+    match optional {
+      None => required,
+      Some(optional) => format!("{} {}", optional, required),
+    }
+  }
+
+  #[napi]
+  pub fn option_start_end(
+    optional1: Option<String>,
+    required: String,
+    optional2: Option<String>,
+  ) -> String {
+    match (optional1, optional2) {
+      (None, None) => required,
+      (None, Some(optional2)) => format!("{} {}", required, optional2),
+      (Some(optional1), None) => format!("{} {}", optional1, required),
+      (Some(optional1), Some(optional2)) => format!("{} {} {}", optional1, required, optional2),
+    }
+  }
+
+  #[napi]
+  pub fn option_only(optional: Option<String>) -> String {
+    match optional {
+      None => "".to_string(),
+      Some(optional) => format!("{}", optional),
+    }
+  }
+}