-
Notifications
You must be signed in to change notification settings - Fork 235
fix: correctly handled mapped union types #2490
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
base: next
Are you sure you want to change the base?
Changes from all commits
9315395
39c6c8f
758f556
5a6affd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -95,6 +95,9 @@ export function isAssignableTo( | |
| inferMap: Map<string, BaseType> = new Map(), | ||
| insideTypes: Set<BaseType> = new Set(), | ||
| ): boolean { | ||
| // Keep original source for infer map so annotations (e.g. @discriminator) are preserved | ||
| const originalSource = source; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again, this is not a copy. |
||
|
|
||
| // Dereference source and target | ||
| source = derefType(source); | ||
| target = derefType(target); | ||
|
|
@@ -115,9 +118,9 @@ export function isAssignableTo( | |
| const infer = inferMap.get(key); | ||
|
|
||
| if (infer === undefined) { | ||
| inferMap.set(key, source); | ||
| inferMap.set(key, originalSource); | ||
| } else { | ||
| inferMap.set(key, new UnionType([infer, source])); | ||
| inferMap.set(key, new UnionType([infer, originalSource])); | ||
| } | ||
|
|
||
| return true; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,15 +1,24 @@ | ||
| import type { BaseType } from "../Type/BaseType.js"; | ||
| import { AnnotatedType } from "../Type/AnnotatedType.js"; | ||
| import { AliasType } from "../Type/AliasType.js"; | ||
| import { DefinitionType } from "../Type/DefinitionType.js"; | ||
| import { ReferenceType } from "../Type/ReferenceType.js"; | ||
|
|
||
| /** | ||
| * Return the new type wrapped in an annotated type with the same annotations as the original type. | ||
| * @param originalType The original type. If this is an annotated type, | ||
| * then the returned type will be wrapped with the same annotations. | ||
| * @param originalType The original type. If this is an annotated type (possibly wrapped in AliasType, | ||
| * DefinitionType, or ReferenceType), then the returned type will be wrapped with the same annotations. | ||
| * @param newType The type to be wrapped. | ||
| */ | ||
| export function preserveAnnotation(originalType: BaseType, newType: BaseType): BaseType { | ||
| if (originalType instanceof AnnotatedType) { | ||
| return new AnnotatedType(newType, originalType.getAnnotations(), originalType.isNullable()); | ||
| } | ||
| if (originalType instanceof AliasType || originalType instanceof DefinitionType) { | ||
| return preserveAnnotation(originalType.getType(), newType); | ||
| } | ||
| if (originalType instanceof ReferenceType && originalType.hasType()) { | ||
| return preserveAnnotation(originalType.getType(), newType); | ||
| } | ||
| return newType; | ||
| } |
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We previously simplified the type instead of keeping the anyOf format. Should we mark this as a breaking change?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, it may be considered a breaking change, but I really don't see the gain in simplifying these Unions. I agree that I don't think this is a breaking change, though. It is in the sense that it changes the generated schemas by fixing them (they were wrong before), but the schemas are more correct now and if anything was previously validating but stops validating due to this change, the validation BEFORE this fix was wrong.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree it's a fix. Let's still make it a minor version bump, not just a patch? We might need to mark this pull request as
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would really like to keep the simplifications where possible. Makes schemas much more readable. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| import { assertValidSchema } from "../../utils"; | ||
| import { test } from "node:test"; | ||
|
|
||
| test( | ||
| "valid-data - type-mapped-union-discriminator-shared-annotation", | ||
| assertValidSchema("type-mapped-union-discriminator-shared-annotation", "*", { jsDoc: "basic", discriminatorType: "open-api" }), | ||
| ); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| /** | ||
| * Regression test: when a @discriminator-annotated union goes through a | ||
| * mapped type and is referenced from TWO different exported types, the | ||
| * annotations object produced by preserveAnnotation is shared between | ||
| * the resulting AnnotatedType instances. | ||
| * | ||
| * Before the fix, `delete annotations.discriminator` in | ||
| * AnnotatedTypeFormatter mutated the shared object, so whichever type | ||
| * was processed first would steal the discriminator from the second. | ||
| */ | ||
|
|
||
| type Fish = { | ||
| animal_type: "fish"; | ||
| found_in: "ocean" | "river"; | ||
| }; | ||
|
|
||
| type Bird = { | ||
| animal_type: "bird"; | ||
| can_fly: boolean; | ||
| }; | ||
|
|
||
| /** @discriminator animal_type */ | ||
| type Animal = Fish | Bird; | ||
|
|
||
| // A simple recursive mapped type that preserves structure but forces | ||
| // the union through tryDistributeUnion → preserveAnnotation. | ||
| type DeepMapped<T extends object> = { | ||
| [P in keyof T]: T[P] extends object ? DeepMapped<T[P]> : T[P]; | ||
| }; | ||
|
|
||
| // Two distinct exported types that both embed the same discriminated union | ||
| // through the same mapped type. They will share the annotations object. | ||
| export type First = DeepMapped<{ pet: Animal; label: string }>; | ||
| export type Second = DeepMapped<{ pet: Animal; tag: number }>; |
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.
This doesn't actually make a copy. You could also still use
annotations. The comment about not mutating is for like 66 now, I guess?