From 8652019c94d30923e1e7afd3d4398f097a4dd2e7 Mon Sep 17 00:00:00 2001 From: JoostK Date: Thu, 20 Jan 2022 23:39:22 +0100 Subject: [PATCH] fix(napi-derive): an `Option` in front of a required parameter is no longer optional in TypeScript This fixes a bug where having a parameter of type `Option` followed by required parameters would incorrectly declare the parameter as option in the TypeScript declaration file, resulting in invalid syntax. --- crates/backend/src/typegen/fn.rs | 123 ++++++++++++++------ examples/napi/__test__/typegen.spec.ts.md | 10 ++ examples/napi/__test__/typegen.spec.ts.snap | Bin 2397 -> 2502 bytes examples/napi/index.d.ts | 10 ++ examples/napi/src/callback.rs | 20 ++++ examples/napi/src/class.rs | 44 +++++++ 6 files changed, 172 insertions(+), 35 deletions(-) diff --git a/crates/backend/src/typegen/fn.rs b/crates/backend/src/typegen/fn.rs index 65a7e2ff..f794f8fe 100644 --- a/crates/backend/src/typegen/fn.rs +++ b/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, + last_required: Option, +} + +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 for FnArgList { + fn from_iter>(iter: T) -> Self { + let args = iter.into_iter().collect::>(); + 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 { 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); - if is_optional { - format!("arg{}?: {}", i, arg) - } else { - format!("arg{}: {}", i, arg) + let (ts_type, is_optional) = ty_to_ts_type(arg, false); + FnArg { + arg: format!("arg{}", i), + ts_type, + is_optional, } }) - .collect::>() - .join(", "), + .collect::(), 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 - .args - .iter() - .filter_map(|arg| match arg { - crate::NapiFnArgKind::PatType(path) => { - if path.ty.to_token_stream().to_string() == "Env" { - return None; - } - let mut path = path.clone(); - // remove mutability from PatIdent - if let Pat::Ident(i) = path.pat.as_mut() { - i.mutability = None; - } - let mut arg = path.pat.to_token_stream().to_string().to_case(Case::Camel); - let (ts_arg, is_optional) = ty_to_ts_type(&path.ty, false); - arg.push_str(if is_optional { "?: " } else { ": " }); - arg.push_str(&ts_arg); + format!( + "{}", + self + .args + .iter() + .filter_map(|arg| match arg { + crate::NapiFnArgKind::PatType(path) => { + if path.ty.to_token_stream().to_string() == "Env" { + return None; + } + let mut path = path.clone(); + // remove mutability from PatIdent + if let Pat::Ident(i) = path.pat.as_mut() { + i.mutability = None; + } + let arg = path.pat.to_token_stream().to_string().to_case(Case::Camel); + let (ts_type, is_optional) = ty_to_ts_type(&path.ty, false); - Some(arg) - } - crate::NapiFnArgKind::Callback(cb) => { - let mut arg = cb.pat.to_token_stream().to_string().to_case(Case::Camel); - arg.push_str(": "); - arg.push_str(&gen_callback_type(cb)); + Some(FnArg { + arg, + ts_type, + is_optional, + }) + } + 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) - } - }) - .collect::>() - .join(", ") + Some(FnArg { + arg, + ts_type, + is_optional: false, + }) + } + }) + .collect::() + ) } fn gen_ts_func_prefix(&self) -> &'static str { diff --git a/examples/napi/__test__/typegen.spec.ts.md b/examples/napi/__test__/typegen.spec.ts.md index 7812868e..2eff2439 100644 --- a/examples/napi/__test__/typegen.spec.ts.md +++ b/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␊ diff --git a/examples/napi/__test__/typegen.spec.ts.snap b/examples/napi/__test__/typegen.spec.ts.snap index 90bb2d66e15ff43a2621be1e8c138b031c62f1f6..d015efb753710c966076a793e1d13029f8f72113 100644 GIT binary patch literal 2502 zcmV;%2|4ybRzViyFuz3Q3;{7E{egAmf+?n&`=xk&yLBk4KCvLB{~8)z80#A=UKzum1PD zXMcEx|9@p1(=nb}-*HgT6GTCQe1L*h?Jr*(F; zdVXO5*xgvhngkJ5iX7Z&77H4k`X52ASKS?Qzd}gNXedQAAzy#pG#hhUiQu0SM$}~F z%g7vGy=|t;>n7>K*2BsJi1LDT2V4>OPeFH~G)j;OVAlHwhpo$Z-5=Yh7u_Zi$;fAM z2GL7Ipt%%aKmDDIL*><)q!q_>@=EELi~f#b@rG$dlpL`XT{%o#5}jn^P_VE&1wfUI z3?Q7VIoMjIfLb0aFdbUj1-t+x)@+aWkA(If2X?f*wEz-s zpnV&90gWP`2E!&nMb>jGkk>opM)D8{;GY7z9OIt@QAwdrV70NEEy+*drVwIWKnVN< z_KKoM^0?46)>T|e$XdovRKze@pv_V~X4Uut>YhSW&r9(nvKWJq!8mzM?#T_rAJTqJ z8gS-})Od?_PFMS(qp7mIr|U<8tmNzgY%ZVwgw zwwCgPFQB_f>45PY*2W5wTh0=1PKg~W18^u~Yo4X_P7?juB+&=q{wd^0!eEcw&?o`N zqcIj;p}9yT)S=A=R0@nd(u1@~)(hpx;J)P|G*(5bdo&)JibBY~?1Lw-*VB`?Jc<-# zJ19){yiGNnZ)E3Ps`MnqOoAbXL|PQTmkIF0&cFws9}>_7#A0JYfpHH!9qvG|+g$QQ zS$)>ZKPqgL-4V%&&lLR~~IPJl32QW(|AMoM1m1xo&)`*pTxOIi{gv7t%knlo*0So+B7;_eh~^i3pvYK(x76WJquvrz`R+!UtUwkADR9mlBC z%GHV@d!3oPo5Y3wyQr!I2<@{)w_lQ<()v>1h-L&*MM+A1%2m`!zO8phtqUW2D=Q9r zm?)=mxH?0Wu@JIQlIjcoVhF%z?k^=8^u)QU&51hYj>#|3bH>hW zU2QfO%~Q^zIYV)QD*)Ibl-rao01$a3GE-Og@qH$N{b zVxCWDQKB~1*S&G!$yD-)nt8bjM@B5pW9Ur53D>Zdbqewn=*$8f<9Wf(X>R}(KWmjC z*02Fc$=Ze?J#(-^HfG?IBH&Ez7P2Q5lb!N#3E9p5XQ7rk1PvYC@FtyQ+08r09jr{_ z>3Rk?uTRD71*MvDp*#isO2DSCAL!zMfm6cM=CzDMK+y*=W^(9ib^v^-dmG!22Y*MQ zOWSx1yn-Z$-wfxa+{N6^Pd9(JTORJ6wq({1ukbd;#X2UcV zSyy-o2`=4QIz*f8x-qSXmR4Hv|KEL_nMZ(hm@Q65fAD}M&f(&7W@?wOP}!37(lUd5QjX%{rL(< z9iFpWyIyghWoE3}5`09X{0(Uyl#u4B-8LCMAE*xDuxUW@+F%P076pfoMRPX1DQ119 z#?z{L!0ujbSum2H-A#c{C1+Ac>p zyIhU%IT^^&2;N?HADLr;Wh42KlmGtvU;i-sDBkTr(h;9w>*{2shj5uu2Bd{Yx>2X* z+SGFUmc`6W1kA8F7|<&tGT4Rj}K0Iv${s4q@wVCsYmF zGO}TiGr))CY&M#`u~{HtOK2IShVnKq!2Hy^#G7&qB&Wvs z2)(--Y#PX!kwxp>(VNc6L8t4af|7yY=8xN3RntHl@9AdIDm?T{l_?dPQ&qLma;^Vm@JXF}O;o8nssDXooC3C)Vo4#K< QXnk1uKjgVy`a>W90N`uHwEzGB literal 2397 zcmV-j38MBvRzV?n{O-(YNo#D*-}Sv#?8*m5@u zud(6GP!gjNIT>BY{j37HFa;F(V=ClRl3aSAxa>snt(EhauJU+b{m} zyJvrRhW~!^`)7ar?Z3Zx`GRN_aiLowSJaOfdGT@uHksL6bvALArdqD%JVWA7NVk1{ zyn1nI0N8yj6HS7MDn$20+{u|;ZghQUGMwO*=4UqL^}3a zl0)=U5oj(2*iU~WlTdk$7HKC5oxD;y;bO3BSiE6c5hcegLst$Hmqa%mI}|MJZV6DO zV*?21Y7VwhE1;Gq3QUO9rLklQ0_}GMm)%wq5qL4xLok?80&va~Ip&JJ+E4raDw!xE zou-lIy*ugAWRMHu;MVEm^8vU>I}APA$`(!H=bw%Iql6cL#G38%!LiWZ)4+~*wiiIc z4Rr28FQ8H6(_qvhsK|O@1@d~A+)5q-0sPP0EW`yTugL?sgYi1nOO5IrFWX@&nAgJ2=`AR z@(6=Ha!aEW9FHcL%7x}4m5|-GHX;9Lip(Jx$$BZt8QiyAgvP2!b)P09lcxwdkOT1K z^=5YRmPe6-7=m_qT9yB+~@GxAFUu-^1q) zejf6{5ViOU7hP`X{M8^c4C;;bD!&4@J$byf>9N+VFbneVFp+mAti8GbwFxV)d!FJy zFekX?>S0|{oh4$U-9x)Fp`E-wM=dW}6`8S+hh-miqSWg?PPMDpGD1!Cg}>o>p?syi zw7Ot|jv1DZ2XG@W+Xi9cwK5u!ZK2GqE5ME z@(c8wv2$D3o6SY@l(T5fP+Z^&0CoiBHX~6hUr+VG^EyK+2F%u9^u#mhC=(vC!gpu4 z^PAf%&dZ9Jru;lg)yDd|7nd6Giq?E)9ukg>Se#?%Ou-4)u$6TR@)YRY0-WG^$EK}X~VYdTzOJo#5 z8(mYAmW`uG?J1<1MDc-XUkvo6Sv-M04tdy-(ooSpJIc1**>7!a4K5kX3NCtmV+K0- zs%?Ktw0hGXGw^=}dPh{#=v0R6S0?w_B0>PzUJaeXewCA7jKQCQ`Dx#2?`kl>sUiL7 ztF2AbSma&d6(qQ9Yw6R>Y}buxJ+!nMGjwV?V+TIteH2ORFwdVIR>%wT!PH_bfin3( zWNhmtr0EZK_XDJ@PbtS&2=FXuxmX1AnwG?el$-hy51L3u;yif={tAg@K=?QYP^=up zA&+o>zQ(5x&)IFCc_ucbR#%y8IYnj*usNF!Qo@k zoDFY^*_^2{jX6S;Kv@EW2^llU5TT_6bFk)t33DXjP{Zprk^&UCn4#@8oLZ#v%?hsg z$mp=P%Q3!)twuOb268-x*~{)Db1blIEI)Gc-+%q{?`9vxyBkV6<}++vovic_E;Gu2 zv@oO_bsDZsEw`^(!pw_+d4ddvGy${VD`F!&5p0R{KW zABdw#%YuD(ee>}6^1Iga=Y6|zbuN5{ zsAwx(&zQZJIeet~{}C{5(2=QqU!3n!%@g^4L`Q7X|C%b$3Ho2V-nVPhPhF1X2%CD% zpmoLoHF(+MH=k*xGC5rmMqt-v*~A;MNh@az>qcviYo&w$I28{Tl{^vXfy~(r>mbbP zMv^g|_^iuVxYu%C%_`b2*QkwI6%HW(d-yAY(>G*1kb?PPBpl6U6sFewhhZ!R{_-JVYfO z!q(kRsTy@;WW%6ffP>|1Hk!TU6V+A 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 diff --git a/examples/napi/src/callback.rs b/examples/napi/src/callback.rs index 2fe4c926..4254faca 100644 --- a/examples/napi/src/callback.rs +++ b/examples/napi/src/callback.rs @@ -6,6 +6,26 @@ fn get_cwd Result<()>>(callback: T) { callback(env::current_dir().unwrap().to_string_lossy().to_string()).unwrap(); } +#[napi] +fn option_end) -> Result<()>>(callback: T) { + callback("Hello".to_string(), None).unwrap(); +} + +#[napi] +fn option_start, String) -> Result<()>>(callback: T) { + callback(None, "World".to_string()).unwrap(); +} + +#[napi] +fn option_start_end, String, Option) -> Result<()>>(callback: T) { + callback(None, "World".to_string(), None).unwrap(); +} + +#[napi] +fn option_only) -> Result<()>>(callback: T) { + callback(None).unwrap(); +} + /// napi = { version = 2, features = ["serde-json"] } #[napi] fn read_file, Option) -> Result<()>>(callback: T) { diff --git a/examples/napi/src/class.rs b/examples/napi/src/class.rs index 53aa221c..a59227ff 100644 --- a/examples/napi/src/class.rs +++ b/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 { + match optional { + None => required, + Some(optional) => format!("{} {}", required, optional), + } + } + + #[napi] + pub fn option_start(optional: Option, required: String) -> String { + match optional { + None => required, + Some(optional) => format!("{} {}", optional, required), + } + } + + #[napi] + pub fn option_start_end( + optional1: Option, + required: String, + optional2: Option, + ) -> 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 { + match optional { + None => "".to_string(), + Some(optional) => format!("{}", optional), + } + } +}