-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Expand anyOf support to also support oneOf #1133
Conversation
Signed-off-by: Lucian <[email protected]>
@glasserc If you have time a review would be great! |
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'm happy to merge this, but I guess there's a larger question about support for anyOf
and whether it should be different from that of oneOf
. I guess conceptually anyOf
could allow a user to match several of the options? That sounds like it would be hard to produce a good UI for.
I'm trying to understand how this will work? I saw the previous PR for anyOf and it looks similar to what is already supported via schema dependencies. There is currently a problem with schema dependencies in that data will stick around when you change selections, which in combination with additionalProperties: false will result in invalid schemas. I feel that is a pretty big bug as the schema form should never generate invalid schemas. That behaviour is more relevant with anyOf and seems to work correctly, however I find the UI a bit confusing as anyOf should be a multi-select that allows input supporting one or more of the sub-schemas at a given time. With respect to oneOf, I believe this solution is correct, but I'm curious as to whether it too will generate an invalid schema if you update more than one of the possible options. If it behaves exactly like anyOf there's a bug as it will generate invalid schemas. |
@jericmason I'm not clear as to what you're referring to; can you make an issue and give an example schema that doesn't work well as you mention? |
@epicfaace I just checked the updated examples and oneOf does indeed work as expected. It won't generate invalid json for the schema. It would be nice if the current implementation of schema dependencies used the same oneOf functionality (I've added a ticket with examples for that). I'm still a little confused about anyOf; my understanding is that anyOf should support one or more of the provided sub-schemas, but in this implementation it will support the base schema and exactly one sub-schema. Maybe I'm missing something here. I really do appreciated these features and feel like they will be powerful additions to this project. |
@jericmason thanks for the ticket, I've added my thoughts to that. You are exactly right about oneOf and anyOf. Note how an example for the playground for oneOf will error when both schemas are the same (because if both schemas are the same, then the form data can't just match one of those two schemas), while the playground for anyOf will not error. Additionally, anyOf can be used with arrays. |
@epicfaace @jericmason The difference between |
@glasserc Can you release a new version with oneOf support ? |
Signed-off-by: Lucian [email protected]
Reasons for making this change
This is a follow-up to #1118 that expands the code to support
oneOf
as well.Checklist
npm run cs-format
on my branch to conform my code to prettier coding style