-
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
fix 3711 anyOf/oneOf remove old fields from previous schema #3714
base: main
Are you sure you want to change the base?
fix 3711 anyOf/oneOf remove old fields from previous schema #3714
Conversation
@@ -137,8 +139,14 @@ export default function sanitizeDataForNewSchema< | |||
}); | |||
|
|||
newFormData = { | |||
...data, |
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.
@cwendtxealth These changes are very similar to #3443 (comment). In that PR I requested the developer to ensure that none of the other issues were regressed. Are you willing to do that legwork?
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.
Yeah I should be able to validate those issues if they have reproducible steps
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.
Just from some initial testing it does look like removing some undefined fields causes some issues with how getDefaultFormState
fills out formData
. Might need to look at a different place to remove the undefined fields.
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.
@cwendtxealth I'm on vacation for a week starting today. My thoughts are that, perhaps we add another flag to the experimental_defaultFormStateBehavior
that deals with this behavior somehow.
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.
Maybe we talk about this at one of the Friday meetings?
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.
That sounds good!
Hello, any update on this fix? Thanks! |
@linpc This is a very tricky problem that @cwendtxealth hasn't quite figured out how to fix yet. |
Hey @cwendtxealth, @heath-freenome , any predictions on when this might get merged? No pressure, but our workaround for this is giving us some performance issues, so we need to decide if it's worth optimizing. |
Hi @michal-kurz , would you mind sharing your workaround? |
thank you all, just a doubt: it will work on conditionally remove a field with "if" without anyOf or oneOf? |
Thanks for contributing this, I believe it would fix my issue raised in #4366 |
Reasons for making this change
Fixes #3711 - removing old undefined fields when switching to different oneOf/anyOf schema
Checklist
npm run test:update
to update snapshots, if needed.