Skip to content
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

Fix for union types of union types in Records. #1926

Closed
wants to merge 2 commits into from

Conversation

dpvball
Copy link

@dpvball dpvball commented Apr 17, 2024

Existing code was just allowing LiteralType through. The fix now puts the types in the queue and saves if it is a LiteralType. If not then it would be an AliasType or UnionType. These items are then added to the queue and processed one at a time.

The reduce function is there to remove any duplicates in case a type is referenced multiple times in varying sub types
type1 = type2 | type3;
type4 = type1 | type2;

type2 would show up twice in the processing of types. so one of the two would need to be removed.

@dpvball dpvball changed the title Fix for union types of union types in Records. Existing code was just… Fix for union types of union types in Records. Apr 17, 2024
Copy link
Member

@domoritz domoritz left a comment

Choose a reason for hiding this comment

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

Shouldn't this be in the union type formatter or parser?

@dpvball
Copy link
Author

dpvball commented Apr 20, 2024

Shouldn't this be in the union type formatter or parser?

@domoritz So I was taking a look at this. I could add this into the UnionNodeParser and modify the UnionType constructor to take in a secondary variable, but then I'd have to store this as a secondary property on the UnionType which seems a bit quirky since that creates two sources of truth for the types even though one is derived from the other. I've instead modified it so that the UnionType class has a method to dynamically take the internal types and flatten them. And I've updated the MappedTypeNodeParser to reference the getFlattenedTypes method instead of the getTypes method. Let me know your thoughts. Thanks for looking into this.

@@ -64,6 +64,7 @@ function assertSchema(
// skip full check if we are not encoding refs
validateFormats: config.encodeRefs === false ? undefined : true,
keywords: config.markdownDescription ? ["markdownDescription"] : undefined,
allowUnionTypes: true,
Copy link
Member

Choose a reason for hiding this comment

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

Let's not add this here.

@domoritz
Copy link
Member

domoritz commented Apr 20, 2024

Thanks for the issue and sending a pull request. I think I refactored it a bit in #1933 (I just worked on this code yesterday so it felt like I could combine the logic here). I think my version is also a bit more correct since it works when some of the literal unions are exported which means they are not just an alias.

Does that look good to you?

@domoritz domoritz closed this Apr 21, 2024
domoritz added a commit that referenced this pull request Apr 21, 2024
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.

2 participants