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: add condition to pick correct schemas for fixed array items in topathschemainternal util #3916

Merged

Conversation

Rozamo
Copy link
Contributor

@Rozamo Rozamo commented Oct 21, 2023

Reasons for making this change

Fixes #3909.

Changes

  • Adds condition to generate correct path schemas for schemas containing fixed array of items in the toPathSchemaInternal util. The items property for these schemas are of array type instead of object. Because the condition for the array type items was not present, the array type schema was being sent into the recursive function as the schema for the form data, which was not matching. Hence empty form data was being generated each time Form's onChange was being called
  • Adds tests for the above change ensuring 100% coverage for the toPathSchema util

Checklist

  • I'm updating documentation
  • I'm adding or updating code
    • I've added and/or updated tests. I've run npm run test:update to update snapshots, if needed.
    • I've updated docs if needed
    • I've updated the changelog with a description of the PR
  • I'm adding a new feature
    • I've updated the playground with an example use of the feature

@Rozamo Rozamo marked this pull request as draft October 21, 2023 10:11
@Rozamo Rozamo changed the title fix: add condition to process fixed array items in topathschema util fix: add condition to process fixed array items in topathschemainternal util Oct 21, 2023
@Rozamo Rozamo marked this pull request as ready for review October 21, 2023 18:35
@Rozamo Rozamo changed the title fix: add condition to process fixed array items in topathschemainternal util fix: add condition to pick correct schemas for fixed array items in topathschemainternal util Oct 21, 2023

const formData = {
str: 'string',
arr: [{ name: 'name1' }, 'name2'],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering whether there should be a console.warn or console.error for this condition?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see that toPathSchema util is mostly used as a step to omit extra data. In that case should we warn the users about the omitted data?

element,
_recurseList
);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the moment, let's add a console.warn() here to let users know that they have unhandled formData. Which means we want to update the test below with a check that console.warn() was called. This can be done with a jest.spyOn()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. Added a message. Let me know if there is a change required.

Copy link
Contributor Author

@Rozamo Rozamo Nov 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@heath-freenome Should this log be moved up a level outside the for loop? We don't know how many items there will be in the array and if there are too many items, it might clutter the console.

@heath-freenome heath-freenome merged commit 53b1989 into rjsf-team:main Oct 28, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants