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

feature #3796 arrayMinItems.populate = never #3809

Merged

Conversation

alekseyBatuhtin
Copy link
Contributor

@alekseyBatuhtin alekseyBatuhtin commented Aug 3, 2023

Reasons for making this change

Hi team! I've already described the problem in the issue below:
#3796

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

@@ -96,6 +96,11 @@ const liveSettingsSelectSchema: RJSFSchema = {
title: 'Ignore minItems unless field is required',
enum: ['requiredOnly'],
},
{
type: 'string',
title: 'Ignore populating arrays even if settled minItems',
Copy link
Member

Choose a reason for hiding this comment

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

This is a bit confusing. I'm not sure what settled minItems refers to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe you will help me with how to describe it properly?
I tried to describe that your formData won't be populated by any default values of the array:

schema:

...
"anyArray": {
  "type": "array",
  "minItems": 1,
  "items": { "type": "string" }
}
...

formData:

{
  "anyArray": []
}

Copy link
Member

@heath-freenome heath-freenome Aug 4, 2023

Choose a reason for hiding this comment

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

Since the legacy behavior choice says Populate remaining minItems with default values I think the following makes sense.

Suggested change
title: 'Ignore populating arrays even if settled minItems',
title: 'Never populate minItems with default values',

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

expect(
computeDefaults(testValidator, schema, {
rootSchema: schema,
rawFormData: { nonRequiredArray: ['raw0'] },
Copy link
Member

Choose a reason for hiding this comment

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

Might I suggest a different value than the default so that we can see which value is actually being chosen?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course. Which one?

Copy link
Member

@heath-freenome heath-freenome Aug 4, 2023

Choose a reason for hiding this comment

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

Suggested change
rawFormData: { nonRequiredArray: ['raw0'] },
rawFormData: { nonRequiredArray: ['raw1'] },

Which may require the expected value below to be updated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@heath-freenome
Copy link
Member

@alekseyBatuhtin Can you resolve the conflicts in the CHANGELOG.md as well?

@@ -96,6 +96,11 @@ const liveSettingsSelectSchema: RJSFSchema = {
title: 'Ignore minItems unless field is required',
Copy link
Member

@heath-freenome heath-freenome Aug 4, 2023

Choose a reason for hiding this comment

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

And to make the text consistent across all choices, please also make this change

Suggested change
title: 'Ignore minItems unless field is required',
title: 'Only populate minItems with default values when field is required',

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

CHANGELOG.md Outdated
@@ -15,6 +15,18 @@ it according to semantic versioning. For example, if your PR adds a breaking cha
should change the heading of the (upcoming) version to include a major version bump.

-->
# 5.13.0
Copy link
Member

Choose a reason for hiding this comment

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

Actually, this can be moved into the 5.12.0 section below since we haven't released it yet

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@heath-freenome heath-freenome merged commit 825566f into rjsf-team:main Aug 4, 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
Development

Successfully merging this pull request may close these issues.

2 participants