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 omitExtraData bugs for nested empties and nonspecified objects #1419

Merged

Conversation

@Tonexus
Copy link
Contributor

Tonexus commented Aug 19, 2019

Reasons for making this change

Fixes #1388 and fixes #1396. Alternative to #1418.

Modifies behavior of toPathSchema in utils.js (afaik the function is only used in omitting the extra data, let me know if this breaks anything else) to create $name key for arrays as well to be consistent with behavior for objects. Modifies behavior of getFieldNames in Form.js to also add a path to the array of paths if that path points to something empty. Refactors the surrounding code to be more concise and, hopefully, readable. Adds tests for omitExtraData and liveOmit.

Checklist

  • I'm updating documentation
  • I'm adding or updating code
    • I've added and/or updated tests
    • I've updated docs if needed
  • I'm adding a new feature
    • I've updated the playground with an example use of the feature
@Tonexus Tonexus changed the title Feature/omit extra data array of empties Fix omitExtraData bugs for nested empties and nonspecified objects Aug 19, 2019
@Tonexus Tonexus mentioned this pull request Aug 19, 2019
0 of 7 tasks complete
@@ -873,43 +873,34 @@ export function toIdSchema(

export function toPathSchema(schema, name = "", definitions, formData = {}) {
const pathSchema = {
$name: name,
$name: name.replace(/\.$/, ""),

This comment has been minimized.

Copy link
@epicfaace

epicfaace Aug 19, 2019

Member

Why is this needed?

This comment has been minimized.

Copy link
@Tonexus

Tonexus Aug 19, 2019

Author Contributor

As a result of refactoring and because the initial call starts with name as an empty string, I ended up with a period prepended to all of the $names, so I swapped it such that the period is appended, then removed when the $name value is set. e.g. here

https://github.com/Tonexus/react-jsonschema-form/blob/ff66b13dd0e74a36394f3e45e546137b024d709e/src/utils.js#L890

changes `${name}.${i}` to `${name}${i}.`.

Now that I think about it, it was honestly was kind of an arbitrary change. If you think the old style is more clear, I can change it back.

This comment has been minimized.

Copy link
@epicfaace

epicfaace Aug 19, 2019

Member

I think you can change it to the old style, unless there are any additional advantages to this new style.

src/utils.js Outdated Show resolved Hide resolved
src/utils.js Show resolved Hide resolved
@epicfaace

This comment has been minimized.

Copy link
Member

epicfaace commented Aug 19, 2019

Modifies behavior of getFieldNames in Form.js to also add a path to the array of paths if that path points to something empty.

My PR #1418 also does this, although the way it does so isn't very good -- it relies on looking for the first "." in the string (which could be an issue with specially-named keys later). Your method seems better!

https://github.com/mozilla-services/react-jsonschema-form/pull/1418/files#diff-07a63ede6fe55fca89398d42da737369R170

Tony Lau added 3 commits Aug 19, 2019
Tony Lau
Tony Lau
@Tonexus

This comment has been minimized.

Copy link
Contributor Author

Tonexus commented Aug 19, 2019

@epicfaace I added your tests, and if I understand correctly, describeRepeated essentially parameterizes the tests and runs the 3 relevant combinations of omitExtraData and liveOmit for the enclosed tests. In using the function in Form_test.js, you repeat all of the current tests for the 3 combinations, right? So then there are no tests to make sure that omitExtraData actually omits the correct data yet, right?

Tony Lau
@epicfaace

This comment has been minimized.

Copy link
Member

epicfaace commented Aug 20, 2019

@epicfaace I added your tests, and if I understand correctly, describeRepeated essentially parameterizes the tests and runs the 3 relevant combinations of omitExtraData and liveOmit for the enclosed tests. In using the function in Form_test.js, you repeat all of the current tests for the 3 combinations, right?

Yes.

So then there are no tests to make sure that omitExtraData actually omits the correct data yet, right?

There are tests at the end of Form_test.js that are not within the describeRepeated block; I believe they include a test that tests that behavior of omitExtraData.

@epicfaace

This comment has been minimized.

Copy link
Member

epicfaace commented Aug 20, 2019

By the way, the test that is failing is due to issues with form defaults when omitExtraData and/or liveOmit are true. I fixed them in the other PR with these changes: https://github.com/mozilla-services/react-jsonschema-form/pull/1418/files#diff-07a63ede6fe55fca89398d42da737369R249-R253

@Tonexus

This comment has been minimized.

Copy link
Contributor Author

Tonexus commented Aug 20, 2019

Yeah I see what you are doing, but I think that that logic can be moved to onChange instead. Right now, you have to recompute pathSchema on submit to fix the bug, but it should really be computed whenever the form data changes, as the paths don't actually change on submit.

@epicfaace

This comment has been minimized.

Copy link
Member

epicfaace commented Aug 20, 2019

@Tonexus ok, but if omitExtraData is true and liveOmit is false, then don't we only need to compute the pathSchema on submit? Computing it during onChange is unnecessary and might slow things down.

@Tonexus

This comment has been minimized.

Copy link
Contributor Author

Tonexus commented Aug 20, 2019

@epicfaace That's a good point, if liveOmit is false, pathSchema probably doesn't need to be computed. However, it looks like pathSchema is already being computed on component creation and on props change without checking for liveOmit, and I wanted to be consistent with those cases. Should I change those cases to check for omitExtraData and liveOmit as well?

Also, right now it looks like pathSchema is not propagated to the state in onChange even if both omitExtraData and liveOmit are true, so I'll go ahead and change it so that pathSchema is propagated there as well.

@epicfaace

This comment has been minimized.

Copy link
Member

epicfaace commented Aug 20, 2019

@epicfaace That's a good point, if liveOmit is false, pathSchema probably doesn't need to be computed. However, it looks like pathSchema is already being computed on component creation and on props change without checking for liveOmit, and I wanted to be consistent with those cases. Should I change those cases to check for omitExtraData and liveOmit as well?

Yes, please do.

Also, right now it looks like pathSchema is not propagated to the state in onChange even if both omitExtraData and liveOmit are true, so I'll go ahead and change it so that pathSchema is propagated there as well.

Sounds good, thanks!

@epicfaace epicfaace mentioned this pull request Aug 20, 2019
4 of 8 tasks complete
@Tonexus

This comment has been minimized.

Copy link
Contributor Author

Tonexus commented Aug 21, 2019

@epicfaace Actually, should pathSchema even be part of the state? It is only computed on submit or, with liveOmit true, whenever formData changes. And then it is only applied directly after those two cases, so it doesn't really need to persist.

On a side note, a lot of the logic in onChange and onSubmit seem redundant, maybe some of it can be refactored in another PR...

Tony Lau added 2 commits Aug 21, 2019
@Tonexus

This comment has been minimized.

Copy link
Contributor Author

Tonexus commented Aug 21, 2019

@epicfaace I'm now having issues with the very last test case in the file.

https://github.com/Tonexus/react-jsonschema-form/blob/d23c6480175cf5eb1c041bdb1b7df67eabd0a515/test/Form_test.js#L2663-L2687

Can you explain the reasoning for that one? It looks like it is correctly applying the new schema to the new formData. Is it supposed to use the new formData with the old schema that then removes the baz property?

@epicfaace

This comment has been minimized.

Copy link
Member

epicfaace commented Aug 21, 2019

@epicfaace Actually, should pathSchema even be part of the state?

Yeah, you're right.

For the very last test case, you're right, I think the test case is wrong. I based it off an original test that tested based on an external (props-based) formData change, which used comp.componentWillReceiveProps. I meant to change the test to be based on an internal (user interaction-based) formData change, using Simulate.change. However, this test (as well as the one before it) mixes up both types of props changes in the same test case. Essentially, I meant to remove the comp.componentWillReceiveProps call, but neglected to.

It may be good to split this up; for each test in the final "Schema and formData updates" describe block, make one test that uses comp.componentWillReceiveProps to test an external formData change and another test that tests Simulate.change to test the internal formData change.

@Tonexus Tonexus marked this pull request as ready for review Aug 21, 2019
@Tonexus

This comment has been minimized.

Copy link
Contributor Author

Tonexus commented Aug 21, 2019

@epicfaace Ok, made those changes. There are now 4 new tests for each combo of omitExtraData and liveOmit, and only one actually omits data on form change. Should be ready for review/merge now.

@epicfaace

This comment has been minimized.

Copy link
Member

epicfaace commented Aug 22, 2019

Oh, I remember why pathSchema was initially part of the state. The idea was that it could be passed on to widgets/fields eventually, so that they would be able to know the "path" of the formData that they refer to. However, given that pathSchema is only computed live when liveOmit is true, there would be no point keeping it in the state for now. We can re-add pathSchema into the state later on (later PR) when also giving access to it for widgets/fields.

Copy link
Member

epicfaace left a comment

Looks good! Just a few more things.

src/utils.js Show resolved Hide resolved
src/components/Form.js Show resolved Hide resolved
src/utils.js Show resolved Hide resolved
if (
schema.hasOwnProperty("items") &&
Array.isArray(formData) &&
formData.length > 0

This comment has been minimized.

Copy link
@epicfaace

epicfaace Aug 22, 2019

Member

This line is probably not necessary (line 885)

@Tonexus

This comment has been minimized.

Copy link
Contributor Author

Tonexus commented Aug 22, 2019

@epicfaace Added most of the requested changes. See review comment.

Copy link
Member

epicfaace left a comment

I think we're good! Thanks for all your work on this.

@epicfaace epicfaace merged commit 4251a2c into rjsf-team:master Aug 22, 2019
6 checks passed
6 checks passed
Header rules - rjsf No header rules processed
Details
Pages changed - rjsf 1 new file uploaded
Details
Redirect rules - rjsf No redirect rules processed
Details
Mixed content - rjsf No mixed content detected
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
netlify/rjsf/deploy-preview Deploy preview ready!
Details
@MatinF

This comment has been minimized.

Copy link

MatinF commented Aug 22, 2019

Happy to do some tests of this to see if it resolves #1396 and #1425 - is the playground link updated?

@epicfaace

This comment has been minimized.

Copy link
Member

epicfaace commented Aug 22, 2019

You can use the playground link for the deploy preview of this PR: https://deploy-preview-1419--rjsf.netlify.com/

@Tonexus Tonexus mentioned this pull request Aug 22, 2019
3 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.