Skip to content

fix(typescript): generate scalar decalration types and tighten union generation to require multiple local subclasses#209

Closed
muhabdulkadir wants to merge 0 commit intoaccordproject:mainfrom
muhabdulkadir:main
Closed

fix(typescript): generate scalar decalration types and tighten union generation to require multiple local subclasses#209
muhabdulkadir wants to merge 0 commit intoaccordproject:mainfrom
muhabdulkadir:main

Conversation

@muhabdulkadir
Copy link
Copy Markdown
Contributor

@muhabdulkadir muhabdulkadir commented Apr 9, 2026

Closes #

This PR updates the TypeScript from-CTO visitor to improve generated type fidelity in two areas:

  • Scalar declarations are now emitted as TypeScript type aliases.
  • Union aliases are now emitted/used only when there are multiple same-namespace subclasses.

Problem

The previous behavior had two gaps:

  • Scalar declarations in models were not emitted as first-class TypeScript aliases, which made scalar fields lose their domain-specific naming in generated output.
  • Union aliases could be produced for single-subclass cases, creating singleton unions that add noise without meaningful polymorphic value.

Changes

  • Scalar declaration generation
    • Added explicit scalar declaration handling to the visitor so they are emitted during model traversal.
      Example behavior:
      Scalar EmailAddress extends String
      
      maps to:
      
      export type EmailAddress = string;
      
  • Scalar field behavior
    • Updated field emission logic so type-scalar fields are written using their scalar alias name (not collapsed to primitive directly).
       Example:
       export type RegistrationDate = Date;
       
       export interface IEntity extends IConcept {
          entityId: string;
          created: RegistrationDate;
       }
      
      
  • Union generation tightening
    • Class-level union alias generation now requires more than one direct subclass in the same namespace.
    • flattenSubclassesToUnion now also requires more than one same-namespace subclass before switching a field type to Union form.
    • Cross-namespace subclasses are excluded from union eligibility, keeping dependency direction clean and avoiding overreach in parent namespace output. FYI: Parent namespaces would no longer import types from child namespace to define unions, and unions would be scoped locally to specific namespace.

Screenshots or Video

Related Issues

  • Issue #
  • Pull Request #

Author Checklist

  • Ensure you provide a DCO sign-off for your commits using the --signoff option of git commit.
  • Vital features and changes captured in unit and/or integration tests
  • Commits messages follow AP format
  • Extend the documentation, if necessary
  • Merging to main from fork:branchname

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates the TypeScript from-CTO code generator to improve type fidelity for Concerto scalars and to reduce noise from singleton union aliases, keeping unions and imports scoped to same-namespace subclasses.

Changes:

  • Emit scalar declarations as exported TypeScript type aliases and preserve scalar names when emitting scalar-typed fields.
  • Tighten class union alias generation and flattenSubclassesToUnion behavior to require multiple same-namespace subclasses (excluding cross-namespace subclasses).
  • Update TypeScript generator tests and Jest snapshots to reflect the new output (fewer subclass imports/warnings, fewer singleton unions, new scalar aliases).

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
lib/codegen/fromcto/typescript/typescriptvisitor.js Adds scalar declaration emission; preserves scalar alias names for fields; restricts union generation to multiple same-namespace subclasses; removes cross-namespace subclass import behavior.
test/codegen/fromcto/typescript/typescriptvisitor.js Updates/extends unit tests for tightened union logic and new scalar generation/field behavior.
test/codegen/fromcto/data/model/comprehensive.cto Adds a comprehensive CTO fixture covering many declaration types (scalars/enums/maps/resources/etc.).
test/codegen/snapshots/codegen.js.snap Updates snapshots for removed singleton unions/warnings and new scalar aliases/import changes.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 122 to 127
if (!properties.has(typeNamespace)) {
properties.set(typeNamespace, new Set());
}
properties.get(typeNamespace).add(property.isTypeEnum?.() ? typeName : `I${typeName}`);
// Scalars and enums use their plain name; classes use the I prefix
properties.get(typeNamespace).add((property.isTypeEnum?.() || property.isTypeScalar?.()) ? typeName : `I${typeName}`);
}
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Map type references are still treated like class/interface references when collecting imports: for non-primitive properties you add I${typeName} unless it’s an enum or scalar. However visitMapDeclaration emits maps as export type ${MapName} = Map<...>, so importing I${MapName} will reference a non-existent export and can break the generated TS (e.g., snapshots show employeeTShirtSizes?: EmployeeTShirtSizes; but imports include IEmployeeTShirtSizes). Consider detecting map declarations (via modelManager.getType(property.getFullyQualifiedTypeName()).isMapDeclaration?.()) and importing the plain typeName like scalars/enums.

Copilot uses AI. Check for mistakes.
@mttrbrts
Copy link
Copy Markdown
Member

Thanks for this contribution! Before we can review, please resolve the merge conflicts with the base branch.

git fetch origin main
git rebase origin/main
# Resolve conflicts
git push --force-with-lease

Once the conflicts are resolved, we'll proceed with the review.


This comment was generated by AI on behalf of @mttrbrts.

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.

4 participants