-
Notifications
You must be signed in to change notification settings - Fork 2.3k
feat: allow override form values with defaults #4625
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?
Conversation
it('should return empty array even when no defaults', () => { | ||
expect(mergeDefaultsWithFormData(undefined, [2], undefined, undefined, 'replace')).toEqual([]); | ||
}); |
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 I'm misunderstanding the feature...why does this return an empty array?
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's because undefined will be treated as missing value for this, so replaced with proper default for arrays (empty). Do you think it's incorrect?
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.
My assumption was that mergeDefaultsWithFormData
treats undefined
computed defaults as there being no defaults, so I would think in this scenario we keep user data. But if it's the case that it's only called when there is some default value (even if it's computed to be undefined
), then it makes sense to replace user data with some value like []
. So I think what you have is fine. We can refine the implementation over time too if necessary.
it('should not return undefined from defaults', () => { | ||
expect( | ||
mergeDefaultsWithFormData({ a: { b: undefined } }, { a: { b: { c: 1 } } }, undefined, undefined, 'replace'), | ||
).toEqual({ | ||
a: { b: {} }, | ||
}); | ||
}); |
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.
undefined
is not a valid JSON value, so what JSON Schema results in a default
that looks like { a: { b: undefined } }
? Are we doing something special in RJSF that I am ignorant of?
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 do not know actually how it might happen in JSON schema, I just tried to repeat existing tests. Here we wiill replace missing value (undefined from default arg) with proper default for object (empty one). Do you think it's incorect?
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.
It looks like our computeDefaults
utility may return undefined
, so I guess we do need to handle it.
@@ -59,6 +59,7 @@ should change the heading of the (upcoming) version to include a major version b | |||
|
|||
- Fixed form data propagation with `patternProperties` [#4617](https://github.com/rjsf-team/react-jsonschema-form/pull/4617) | |||
- Updated the `GlobalUISchemaOptions` types to extend `GenericObjectType` to support user-defined values for their extensions | |||
- Allow form value overrides with defaults [#4625](https://github.com/rjsf-team/react-jsonschema-form/pull/4625 |
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.
This needs to move to a new # 6.0.0-beta.11
section since we released this one already
@Lonli-Lokli it looks like you and another person are making changes to the exact same code, can you try out the other person's fix to see if that solves your problem? If not, I'm tempted to merged theirs first since it does not involve adding a new feature and then you can apply your changes to it? |
@heath-freenome I can wait until #4637 be merged, rebase and recheck. I do not think this PR will solve my issue with array defaults, actually. |
Reasons for making this change
Add the ability to override existing form values (especially arrays) with defaults
Checklist
npx nx run-many --target=build --exclude=@rjsf/docs && npm run test:update
to update snapshots, if needed.