Skip to content

Conversation

@oscartbeaumont
Copy link
Member

Bring back the Zod exporter as it was lost in a refactor.

@oscartbeaumont
Copy link
Member Author

The initial pass of this was AI generated as it can refer to both the old Zod code & Typescript exporter so it should be straight forward.

This is the AI review on it's own work:

# Specta Zod Exporter - Code Review

## Critical Issues

### 1. **Missing Serde Integration Flag**
- **Location**: `lib.rs:72-76`, `export()` method
- **Issue**: The TypeScript exporter has a `with_serde()` method and `serde: bool` field to conditionally enable Serde validation, but the Zod exporter always calls `specta_serde::validate(types)?` unconditionally
- **Impact**: Users cannot opt out of Serde validation
- **Fix**: Add a `serde: bool` field to the `Zod` struct and a `with_serde()` method, then conditionally call validation

### 2. **Optional Field Handling Incorrect**
- **Location**: `lib.rs:366-370`, `lib.rs:532-536`
- **Issue**: Using `.optional()` in Zod is incorrect. In Zod, optional fields use `.optional()` which makes the field itself optional (can be omitted), but `Option<T>` should map to `.nullable()` which allows `null` values
- **TypeScript Comparison**: The TS exporter uses `field?:` for optional fields and `| null` for nullable types
- **Zod Semantics**:
  - `z.string().optional()` = field can be omitted (undefined)
  - `z.string().nullable()` = field can be null
  - `z.string().nullable().optional()` = field can be omitted or null
- **Impact**: Runtime validation will fail for properly serialized JSON with null values
- **Fix**: Remove the `.optional()` suffix for fields with `field.optional()` - the optionality should be handled at the TypeScript type level, not the Zod schema level

### 3. **Generics Implementation Incorrect**
- **Location**: `lib.rs:182-186`, `lib.rs:265-272`
- **Issue**: The implementation uses TypeScript-style generic syntax `<T>` for Zod schemas, but Zod schemas are runtime validators, not types. Zod doesn't support generics in the same way TypeScript does
- **Example**: `export const User<T> = z.object(...)` is invalid JavaScript
- **Impact**: Generated code will have syntax errors
- **Fix**: For generic types, either:
  1. Export them as functions that return schemas: `export const User = <T>(tSchema: z.ZodType<T>) => z.object(...)`
  2. Don't support generics (simpler approach)
  3. Inline all generic usages (most practical)

### 4. **Reference with Generics Implementation Invalid**
- **Location**: `lib.rs:265-272`
- **Issue**: `format!("{}({})", ndt.name(), generics)` generates invalid Zod syntax like `User(z.string())`
- **Impact**: Generated schemas are not valid Zod code
- **Fix**: Need to design how to handle generic type references in Zod context

### 5. **Missing Import Statement for Zod**
- **Location**: `lib.rs` export output
- **Issue**: The generated file doesn't include `import { z } from 'zod'` at the top
- **Impact**: Generated TypeScript file won't work without manual addition of the import
- **Fix**: Add an import statement in the header/framework_header

## Major Issues

### 6. **Flattened Fields Implementation Incomplete**
- **Location**: `lib.rs:349-353`, `lib.rs:376-387`
- **Issue**: Flattened fields are handled but the logic is incomplete:
  1. When a field is flattened, the schema is pushed to `field_sections` but not properly merged
  2. The current implementation would generate `.and()` chains but doesn't unwrap the flattened object
- **Example**: For `#[serde(flatten)]`, Zod needs `.merge()` or proper spreading, not just `.and()`
- **Impact**: Flattened structs won't validate correctly
- **Fix**: Use `.merge()` for Zod schema intersection when dealing with flattened fields

### 7. **Empty Flattened Fields Edge Case**
- **Location**: `lib.rs:376-387`
- **Issue**: If `field_sections` is not empty but `unflattened_fields` is empty, the code doesn't handle this case properly - it would generate incomplete schemas
- **Impact**: Some edge cases with only flattened fields would generate invalid schemas
- **Fix**: Add proper handling for when only flattened fields exist

### 8. **Duplicate Schema Deduplication Missing**
- **Location**: `lib.rs:408-476` (enum_to_zod)
- **Issue**: The TypeScript exporter has `variants.dedup()` to remove duplicate variant schemas, but the Zod implementation doesn't
- **Impact**: May generate redundant union members like `z.union([z.null(), z.null()])`
- **Fix**: Add deduplication of variant schemas before joining

### 9. **Reserved Keywords Check Missing**
- **Location**: `lib.rs:564-588` (sanitise_type_name)
- **Issue**: The TypeScript exporter checks against `RESERVED_TYPE_NAMES` but Zod exporter doesn't
- **Impact**: May generate schemas with names that conflict with JavaScript/TypeScript reserved words
- **Fix**: Import and check against reserved names like the TypeScript exporter does

### 10. **Test Types Not Implementing NamedType**
- **Location**: `tests/lib.rs` - all test types
- **Issue**: All test structs/enums use `#[specta(export = false)]` which prevents them from implementing `NamedType` trait
- **Impact**: Tests don't compile
- **Fix**: Remove `#[specta(export = false)]` or use a different approach for test-only types

## Minor Issues

### 11. **Unused Context Fields**
- **Location**: `context.rs:14-17`
- **Issue**: `cfg` and `is_export` fields in `ExportContext` are never read
- **Impact**: Dead code warnings, potentially missing functionality
- **Fix**: Either use these fields (likely in future features) or remove them

### 12. **Missing Documentation on Public Items**
- **Location**: Multiple (Error variants, public exports)
- **Issue**: Some public items lack documentation comments
- **Impact**: Poor IDE experience for users
- **Fix**: Add doc comments, especially for `Error` and `ExportPath`

### 13. **Inconsistent String Literal Escaping**
- **Location**: `lib.rs:214-215`, `lib.rs:547-561`
- **Issue**: String literals aren't escaped for special characters (quotes, newlines, etc.)
- **Example**: If a user has `#[serde(rename = "has\"quote")]`, the generated Zod will be invalid
- **Impact**: Generated schemas may be syntactically invalid for certain edge cases
- **Fix**: Properly escape string values in literal and key sanitization

### 14. **Typo in Documentation**
- **Location**: `lib.rs:97`
- **Issue**: "unless your a framework" should be "unless you're a framework"
- **Impact**: Minor documentation quality issue
- **Fix**: Correct typo

### 15. **Missing JSDoc/Comments Export**
- **Location**: Entire implementation
- **Issue**: TypeScript exporter has JSDoc support for exporting Rust doc comments, but Zod exporter doesn't
- **Impact**: Users lose documentation when exporting
- **Fix**: Add comment/JSDoc export support similar to TypeScript exporter

### 16. **No Support for `#[specta(inline)]` Attribute**
- **Location**: Missing from implementation
- **Issue**: The TypeScript exporter respects `#[specta(inline)]` to inline types instead of referencing them, but Zod implementation doesn't check for this
- **Impact**: May generate different output than expected by users familiar with the TypeScript exporter
- **Fix**: Check `field.inline()` and inline the type when true

## Design Questions

### 17. **Generics Strategy Undefined**
- **Question**: How should generic types be exported in Zod?
- **Options**:
  1. Export as functions that accept Zod schemas
  2. Don't support generics (error on generic types)
  3. Always inline generic usages
  4. Generate factory functions
- **Recommendation**: Discuss with users/maintainers before implementing

### 18. **Format Support Missing**
- **Issue**: TypeScript exporter supports multiple formats (Namespaces, Files, ModulePrefixedName, FlatFile), but Zod only supports a single flat file
- **Impact**: Limited compared to TypeScript exporter
- **Question**: Should Zod support multiple formats? Zod schemas are typically in a single file, so this may be intentional

### 19. **Missing Validation Methods**
- **Issue**: TypeScript has `specta_serde::validate()` integration, but no public API for validation-only operations
- **Question**: Should there be an `export_with_validation()` vs `export()` distinction?

## Positive Observations

1. **Good error handling structure** - Using `thiserror` and proper error types
2. **Consistent API with TypeScript exporter** - Builder pattern matches well
3. **Comprehensive type coverage** - All DataType variants are handled
4. **Path tracking for errors** - Good developer experience when errors occur

## Recommendations Priority

**Must Fix (Breaks Functionality):**
1. Issue #3 - Generics syntax
2. Issue #4 - Reference with generics
3. Issue #5 - Missing Zod import
4. Issue #10 - Test compilation

**Should Fix (Correctness):**
5. Issue #2 - Optional field semantics
6. Issue #6 - Flattened fields
7. Issue #8 - Deduplication
8. Issue #9 - Reserved keywords

**Nice to Have:**
9. Issue #1 - Serde flag
10. Issue #13 - String escaping
11. Issue #15 - JSDoc support
12. Issue #16 - Inline support

**Code Quality:**
13. Issue #11 - Dead code
14. Issue #12 - Documentation
15. Issue #14 - Typos

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants