-
Notifications
You must be signed in to change notification settings - Fork 460
Support records in KDL #2361
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: main
Are you sure you want to change the base?
Support records in KDL #2361
Conversation
|
Thanks for the PR! This section of the codebase is owned by @saschanaz - if they write a comment saying "LGTM" then it will be merged. |
src/build/patches.ts
Outdated
| subtype: subType, | ||
| ...optionalMember("type", "string", typeValue), | ||
| subtype: | ||
| typeValue == "record" && typeof subType?.type !== "string" |
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.
I think this shouldn't check the type name. We could use this for any parameterized type, right?
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.
I think we should, when I removed that check, it broke the output to be:
readonly links: HTMLCollectionOf<HTMLAnchorElement, HTMLAreaElement>;
Instead of the original:
readonly links: HTMLCollectionOf<HTMLAnchorElement | HTMLAreaElement>;
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.
Oh no, ambiguate way to pass second type vs union. We need to disambiguate it rather than using the same way here then.
|
What do you think of the idea of adding the union property, like how I did it in the last commit? |
|
How do we handle |
I have searched the JSONC files and there is nothing like this. If there is an extremely rare case when this is needed, we can use overrideType |
No description provided.