fix: dereference alias types in discriminated unions (#1750)#2420
fix: dereference alias types in discriminated unions (#1750)#2420CristianLopezR wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes discriminated-union discriminator extraction when the discriminator property type is wrapped (e.g., via exported type aliases), by dereferencing the discriminator property type before generating if/then branches.
Changes:
- Dereference discriminator property types in
UnionTypeFormatter.getJsonSchemaDiscriminatorDefinition()to correctly extract underlying literal values. - Add a new valid-data fixture and test case intended to cover alias-type dereferencing for discriminated unions.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/TypeFormatter/UnionTypeFormatter.ts |
Dereferences discriminator property type before definition generation. |
test/valid-data/dereference-alias-types-discriminated-unions/main.ts |
Adds a TypeScript fixture with alias-based discriminator property types. |
test/valid-data/dereference-alias-types-discriminated-unions/schema.json |
Adds the expected schema output for the new fixture. |
test/valid-data/dereference-alias-types-discriminated-unions/index.test.ts |
Registers the new valid-data schema assertion test. |
Comments suppressed due to low confidence (1)
src/TypeFormatter/UnionTypeFormatter.ts:55
kindTypesis built from a filteredtype.getTypes()list (excluding types that deref toNeverType), but the error message usestype.getTypes()[undefinedIndex]from the unfiltered list. If any union member dereferences toNeverType, the index can point at the wrong member type in the error. Consider keeping aconst filteredTypes = ...array and using it consistently for bothkindTypesand error reporting.
const kindTypes = type
.getTypes()
.filter((item) => !(derefType(item) instanceof NeverType))
.map((item) => {
const propertyType = getTypeByKey(item, new LiteralType(discriminator));
return propertyType ? derefType(propertyType) : undefined;
});
const undefinedIndex = kindTypes.findIndex((item) => item === undefined);
if (undefinedIndex !== -1) {
throw new JsonTypeError(
`Cannot find discriminator keyword "${discriminator}" in type ${type.getTypes()[undefinedIndex].getName()}.`,
type,
);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| export type MyUnion = ObjectA | ObjectB; |
There was a problem hiding this comment.
This fixture is named/used as a discriminated-union regression test, but MyUnion is not annotated with @discriminator kind, so UnionTypeFormatter.getJsonSchemaDiscriminatorDefinition() (and the new deref logic) will never run. Add a JSDoc @discriminator kind on MyUnion to actually exercise the discriminated-union code path.
There was a problem hiding this comment.
This is right, the PR attempts to fix discriminated unions but your union is not discriminated
| "MyUnion": { | ||
| "anyOf": [ | ||
| { |
There was a problem hiding this comment.
If this is meant to validate discriminated-union output, the expected schema should reflect the @discriminator handling (i.e., a definitions.MyUnion object with allOf containing if/then branches and a discriminator properties.kind.enum). The current expected schema is a plain anyOf of the variants, which does not exercise getJsonSchemaDiscriminatorDefinition() or the new dereferencing behavior.
| @@ -0,0 +1,14 @@ | |||
| type KindA = "kind_a"; | |||
| type KindB = "kind_b"; | |||
There was a problem hiding this comment.
what happens if you export KindB? will it keep being a reference instead of inlining like
{
"const": "kind_a",
"type": "string"
},?
This PR fixes issue #1750.
Previously,
getJsonSchemaDiscriminatorDefinitionfailed to identify literal types when they were wrapped in a Type Alias (e.g.,type MyKind = "kind_a").This change adds a
derefType()step when retrieving the discriminator property type inUnionTypeFormatter.ts, ensuring the underlying literal value is correctly extracted.Fixes #1750