fix: expose referred DefinitionType if it wraps an AliasType #1047
Conversation
| @@ -16,8 +17,12 @@ export class ReferenceTypeFormatter implements SubTypeFormatter { | |||
| return { $ref: `#/definitions/${this.encodeRefs ? encodeURIComponent(ref) : ref}` }; | |||
| } | |||
| public getChildren(type: ReferenceType): BaseType[] { | |||
| if (type.getType() instanceof DefinitionType) { | |||
| return []; | |||
| const referredType = type.getType(); | |||
There was a problem hiding this comment.
I think it goes too deep in the hierarchy. It returns the UnionType but we need the DefinitionType that wraps it.
There was a problem hiding this comment.
Are you saying we need the last definition type right above the non definition type? Maybe it's with pulling that functionality into a until function. We could add a flag to the deref function.
There was a problem hiding this comment.
So, we have a piece of path from a RefType to a UnionType:
ReferenceType -> DefinitionType -> AliasType -> UnionType
Originally the traversal didn't reach the DefinitionType or its children, but we need to return a/the DefinitionType that wraps the UnionType. The intervening AliasType is a bit mystery to me. I guess we could also return an instance of a new DefinitionType that wraps the the UnionType directly(?) Anyway, I'm not sure which are exactly the situations where the traversal in ReferenceTypeFormatter should proceed to the DefinitionType. Now I used the presence of an AliasType as an indication for that. Although it works, it may not make much sense.
It actually seems that we could always traverse the DefinitionType. Tests seem to pass. I just thought that there was some reason for stopping propagation there. Maybe it was again an optimization that isn't needed any more?
There was a problem hiding this comment.
If we can just not have this check here, let's delete it. I don't understand the logic otherwise ;-)
There was a problem hiding this comment.
Hmm I spent some time stepping through the execution but didn't see the obvious bug yet. I added a breakpoint in ReferenceTypeFormatter.ts and then debugged your new test case and the existing test cases. I then set the code to generate the schema files from the tests and disabled the reachability check.
Alternatively, I created a test file and then ran yarn run run -p test.ts -t Intersection
I'm not satisfied with the hack above tbh.
The alias type makes sense. It's the union.
It goes away if you inline the type.
I would really like to understand why we don't format the definitions for the children.
There was a problem hiding this comment.
I have a hunch what's happening.
We can expand the Intersection type to export type Intersection = (A & C) | (B & C);. See #985 and d9b9a15.
This means, we think that we don't need the Union type anymore, since we transformed it in d9b9a15#diff-79b658f531e0da963f97358d1c4ab65d18674a91e460f86500cca7cbdee966b3R23.
We addressed part of this for aliases that are inside the union but not for the union itself in https://github.com/vega/ts-json-schema-generator/pull/985/files.
So I think the fix needs to be in the Intersection/Union code, not the reference type.
There was a problem hiding this comment.
We return [] because we know that definitions should already have been created. They usually already are but here is the exception.
There was a problem hiding this comment.
Hmm, or maybe we can just return this.childTypeFormatter.getChildren(referredType) instead of doing the check. Maybe we do this because we think the definition is already through some other way defined. However, since we do caching and detect duplicated, I think we should be fine just returning the children.
There was a problem hiding this comment.
Thank you for spending time on this issue/pr!
| y: number; | ||
| } | ||
|
|
||
| export type Union = Container | Silly; |
There was a problem hiding this comment.
Let's just test two things. First, what if we inline this type or not export it. Second, what if we define another type that just aliases Union?
You can test it locally. No need to write a test case unless you notice that it fails.
There was a problem hiding this comment.
Sure. I tested various combinations of inlining, not exporting, aliasing, etc. No missing definitions occurred and the produced schemas looked okay to me.
There was a problem hiding this comment.
Hi @domoritz! What do you think, is there still something else to test or adjust?
There was a problem hiding this comment.
No. I need to do a review. Thank you for your patience while I come back from the break and need to catch up on some other things.
I bumped up this review in my queue.
|
Moved to #1093. |
Closes #1046.
The
UnionTypeobject representing theUniontype in the example was buried deep in the type hierarchy. ThegetChildren()method ofReferenceTypeFormatterignoredDefinitionTypes and thus, didn't to expose the hierarchically referred type.This PR exposes the referred
DefinitionTypeif it wraps anAliasType. This seems to work, but I'm not 100% sure that this is the proper way to fix this – mainly because understanding the purpose of different types is quite difficult. However, all tests pass.