Skip to content

Conversation

bennypowers
Copy link

@bennypowers bennypowers commented Jul 27, 2025

Note

This PR was written with the help of LLM code assist tools. Please let me know if that's against repo policy and I'll close it.

Allows discriminator to operate on nested unions, and on unions in which some members lack the discriminating field entirely.

Enable discriminators on unions where some members lack the discriminator field (non-congruent discriminators)

Background

This PR addresses a critical issue encountered when generating JSON schemas for the custom-elements-manifest project, specifically related to discriminated unions in TypeScript where not all union members contain the discriminator field.

Related Issue: custom-elements-manifest #123

Problem Statement

The custom-elements-manifest project defines a complex type hierarchy for representing web component metadata:

/**
 * @discriminator kind
 */
export type Declaration = 
    | FunctionDeclaration      // kind: 'function'
    | VariableDeclaration      // kind: 'variable'  
    | MixinDeclaration         // kind: 'mixin'
    | CustomElementMixinDeclaration // kind: 'mixin', customElement: true
    | ClassDeclaration         // kind: 'class'
    | CustomElementDeclaration // kind: 'class', customElement: true

This represents a hierarchical discriminator pattern where:

  1. Primary discriminator: kind field distinguishes the base type categories
  2. Secondary discriminator: customElement field distinguishes between regular and custom element variants
  3. Non-congruent structure: Some types (CustomElementMixinDeclaration, CustomElementDeclaration) have additional fields that others lack

The Core Issue

Previously, ts-json-schema-generator would fail with this error:

Duplicate discriminator values: mixin, class in type "(FunctionDeclaration|VariableDeclaration|MixinDeclaration|CustomElementMixinDeclaration|ClassDeclaration|CustomElementDeclaration)".

This occurred because:

  • Both MixinDeclaration and CustomElementMixinDeclaration share kind: "mixin"
  • Both ClassDeclaration and CustomElementDeclaration share kind: "class"
  • The generator couldn't distinguish between them and threw a "duplicate discriminator values" error

Why This Pattern Matters

The custom-elements-manifest schema needs to represent the reality of web component development:

  1. Regular classes/mixins: Standard TypeScript constructs (kind: "class", kind: "mixin")
  2. Custom element classes/mixins: Enhanced with web component metadata (kind: "class" + customElement: true)

This hierarchical structure allows tools to:

  • Filter for only custom element-related declarations using customElement: true
  • Process all class-like declarations using kind: "class"
  • Handle the inheritance relationship where custom elements extend regular classes

Solution

This PR implements support for non-congruent and hierarchical discriminated unions by:

1. Non-Congruent Discriminator Support

Handles unions where some members lack the discriminator field entirely:

/**
 * @discriminator kind  
 */
type Mixed = WithKind | WithoutKind; // Some types don't have 'kind' field

2. Hierarchical Discriminator Logic

For types sharing discriminator values, generates sophisticated conditional schemas:

{
  "allOf": [
    {
      "if": {
        "allOf": [
          { "properties": { "kind": { "const": "class" } }, "required": ["kind"] },
          { "not": { "properties": { "customElement": {} }, "required": ["customElement"] } }
        ]
      },
      "then": { "$ref": "#/definitions/ClassDeclaration" }
    },
    {
      "if": {
        "properties": { 
          "kind": { "const": "class" }, 
          "customElement": { "const": true } 
        },
        "required": ["kind", "customElement"]
      },
      "then": { "$ref": "#/definitions/CustomElementDeclaration" }
    }
  ]
}

3. Smart Error Detection

Still properly fails for genuinely invalid discriminators:

// This still throws an error - both types have same discriminator but no distinguishing fields
type Invalid = 
  | { type: "A"; field1: string }  
  | { type: "A"; field2: number }; // ❌ Can't distinguish these

Implementation Details

Key Changes

  1. UnionTypeFormatter.ts: Modified getJsonSchemaDiscriminatorDefinition() to:

    • Separate types with/without discriminator fields
    • Group types by discriminator value
    • Generate hierarchical conditions using secondary fields
    • Maintain error detection for truly invalid cases
  2. Conditional Schema Generation: Uses JSON Schema if/then/else with:

    • Positive conditions for types with discriminator + secondary fields
    • Negative conditions (not) for types lacking secondary fields
  3. Heuristic-Based Detection: Uses type name patterns (e.g., CustomElement*) to identify hierarchical relationships

Test Coverage

  • Basic discriminated unions (existing functionality)
  • Non-congruent discriminators - types without discriminator field
  • Hierarchical discriminators - duplicate primary values with secondary fields
  • Invalid discriminators - still properly fail
  • Integration tests - real-world custom-elements-manifest scenarios

Impact

Before (❌ Broken)

> ts-json-schema-generator --path schema.d.ts --type Package
Error: Duplicate discriminator values: mixin, class in type "..."

After (✅ Working)

> ts-json-schema-generator --path schema.d.ts --type Package
✓ Generated valid JSON Schema with hierarchical discriminator support

Validation

The fix has been validated against the actual custom-elements-manifest test suite:

  • ✅ Valid manifests now pass schema validation
  • ✅ Invalid manifests fail with correct, specific errors
  • ✅ All discriminator test cases pass
  • ✅ No regressions in existing functionality

This enables the custom-elements-manifest project to successfully generate JSON schemas for their complex type hierarchy, unblocking schema-based validation and tooling for the web components ecosystem.

Allows discriminator to operate on nested unions, and on unions in which
some members lack the discriminating field entirely.
@bennypowers
Copy link
Author

Opportunities to improve:

  • I've noticed I get duplicate validation errors for ClassMember without required prop name: { "kind": "field" }. It correctly validates as missing name, but also tries to tell me it should have "kind": "method"

@domoritz domoritz requested a review from arthurfiorette July 28, 2025 09:55
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.

1 participant