- 
                Notifications
    
You must be signed in to change notification settings  - Fork 13
 
Fix #84 #96
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix #84 #96
Conversation
          
WalkthroughIntroduces a recursive  Changes
 Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 
 Possibly related issues
 Poem
 Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
 ✅ Passed checks (3 passed)
 ✨ Finishing touches
 🧪 Generate unit tests (beta)
 Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment   | 
    
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/types/mailtrap.ts (3)
29-36: Good recursive model; consider JSONnulland readonly support.The recursive union looks right and solves #84. Two optional enhancements:
- Include
 null(JSON supports it and templates may emit it).- Accept readonly arrays/objects to ease usage with
 as const.Example minimal tweak:
export type TemplateValue = | string | number | boolean + | null | TemplateValue[] - | { [key: string]: TemplateValue }; + | { [key: string]: TemplateValue }; +// If desired, a more permissive variant: +// | ReadonlyArray<TemplateValue> +// | { readonly [key: string]: TemplateValue };If the platform never returns
null, feel free to skip it. Otherwise, addingnullavoids downstream casts. Can you confirm expected payloads?
111-112: Type alignment: ✅ Also, unifycustom_variablestypes for consistency.Switching to
TemplateVariableshere is correct. Noted inconsistency: several places still useRecord<string, string>forcustom_variableswhileCustomVariablesis broader.Unify to the existing alias:
interface TemplateBatchSendBase extends Omit<Mail, "to"> { from: BaseAddress; template_uuid: string; // Required for template usage template_variables?: TemplateVariables; - custom_variables?: Record<string, string>; + custom_variables?: CustomVariables; reply_to?: BaseAddress; }Same applies to
InlineBatchSendBase(Line 103) andBatchSendRequest.requests(Line 129). Keeps the surface coherent and avoids silent narrowing.
128-129: Unify batch request custom_variables to use CustomVariables type for consistency.The
CustomVariablestype is already defined in the file asRecord<string, string | number | boolean>and is used in the mainBatchSendRequest.requests.custom_variables(line 129) ensures consistent typing across the API and enables complex variable values in per-request overrides.Note: The same type inconsistency exists in
InlineBatchSendBase(line 103) andTemplateBatchSendBase(line 112), which also useRecord<string, string>and should be unified for complete API consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/types/mailtrap.ts(3 hunks)src/types/transport.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/types/transport.ts (1)
src/types/mailtrap.ts (1)
TemplateVariables(36-36)
🔇 Additional comments (1)
src/types/transport.ts (1)
39-39: LGTM:templateVariablesnow matches the canonicalTemplateVariablestype.Verification confirmed that
TemplateVariablesis exported fromsrc/types/mailtrap.tsand properly re-exported to consumers via the main entry point throughexport * from "./types/mailtrap"insrc/index.ts. This aligns transport options with the recursive type definition and removes the need foranycasts. No breaking change expected.
Motivation
Fixed the type mismatch between Mailtrap's template platform and the Node.js SDK. The Mailtrap template platform supports JSON attributes with arrays and nested objects, but the SDK's
TemplateVariablestype only allowed primitive values (string | number | boolean). This prevented users from using the auto-generated JSON from the Mailtrap template platform directly, forcing them to cast toanyas a workaround. Fixes #84Changes
TemplateValuetype definition insrc/types/mailtrap.tsthat supports:string | number | booleanTemplateValue[]{ [key: string]: TemplateValue }TemplateVariablestype to useRecord<string, TemplateValue>instead ofRecord<string, string | number | boolean>TemplateBatchSendBase.template_variablesfromRecord<string, string>toTemplateVariablesinsrc/types/mailtrap.tsBatchSendRequest.requests[].template_variablesfromRecord<string, string>toTemplateVariablesinsrc/types/mailtrap.tsMailtrapMailOptions.templateVariablesfromRecord<string, string | number | boolean>toTemplateVariablesinsrc/types/transport.tsHow to test
npm run lint:tscto verify TypeScript compilation passes without errorsnpm testto ensure all 238 existing tests pass (backward compatibility)template_variables: { user_name: "John", age: 30 }template_variables: { x: { y: "value", z: 123 } }template_variables: { tags: ["tag1", "tag2"] }template_variables: { order: { id: "123", items: [{ name: "Product", price: 29.99 }] } }Summary by CodeRabbit