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: union types of union types in Records #1933

Merged
merged 4 commits into from Apr 21, 2024
Merged

fix: union types of union types in Records #1933

merged 4 commits into from Apr 21, 2024

Conversation

domoritz
Copy link
Member

@domoritz domoritz commented Apr 20, 2024

Revised version of #1926 by dpvball

📦 Published PR as canary version: 2.1.1--canary.1933.1cab987.0

✨ Test out this PR locally via:

npm install ts-json-schema-generator@2.1.1--canary.1933.1cab987.0
# or 
yarn add ts-json-schema-generator@2.1.1--canary.1933.1cab987.0

@dpvball
Copy link

dpvball commented Apr 21, 2024

@domoritz Much more eloquent solution than my PR. Didn't even notice the existence of the derefType utility.

Few comments:

  • I tested it with your code. It looks like it only fails when you have a duplicate reference like we have in the test example. Since MyType3 references MyType2 and MyType2 is already referenced in MyType10. So s2,s3 will show up multiple times in the flattenedType array. I believe you will need to include the duplicate removal code after the LiteralType filter in MappedTypedNodeParser. Not sure if you have a utility function that can more cleanly remove duplicates. But i'll just post what i had before there.
  • I added the allowUnionType since running the tests result in this warning. If that's fine. no issues leaving that out. strict mode: use allowUnionTypes to allow union type keyword at "#/properties/nullableValue" (strictTypes)
  • I think you missed bringing over the test in test/valid-data-type.test.ts

thanks again for all the help on this. I use union types in records extensively in my code and this will help greatly.

@@ -103,7 +103,7 @@ export class MappedTypeNodeParser implements SubNodeParser {

protected getProperties(node: ts.MappedTypeNode, keyListType: UnionType, context: Context): ObjectProperty[] {
return keyListType
.getTypes()
.getFlattenedTypes(derefType)
.filter((type): type is LiteralType => type instanceof LiteralType)
.map((type) => [type, this.mapKey(node, type, context)])
Copy link

Choose a reason for hiding this comment

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

Suggested change
.map((type) => [type, this.mapKey(node, type, context)])
.reduce((acc: LiteralType[], curr: LiteralType) => {
if (
acc.findIndex((val: LiteralType) => {
return val.getId() === curr.getId();
}) < 0
) {
acc.push(curr);
}
return acc;
}, [])
.map((type) => [type, this.mapKey(node, type, context)])

Copy link
Member Author

Choose a reason for hiding this comment

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

We can use uniqueTypeArray here instead I think.

@domoritz domoritz enabled auto-merge (squash) April 21, 2024 14:32
@domoritz domoritz merged commit 0d9c3fb into next Apr 21, 2024
2 of 3 checks passed
@domoritz domoritz deleted the dom/union-records branch April 21, 2024 19:31
@dpvball
Copy link

dpvball commented Apr 22, 2024

thanks for looking into this so quickly @domoritz

@github-actions github-actions bot added the released This issue/pull request has been released. label Apr 23, 2024
Copy link

github-actions bot commented May 6, 2024

🚀 PR was released in v2.1.2-next.0 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
prerelease released This issue/pull request has been released.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants