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

Basic support for 'nullable' types #1213

Merged
merged 6 commits into from
Mar 12, 2019

Conversation

wms
Copy link
Contributor

@wms wms commented Mar 8, 2019

Reasons for making this change

Modifies the implementation of getSchemaType so that when a property has a type of [<any>, 'null'], then <any> will be passed. This allows the library to render and validate the field correctly. For the sake of simplicity, this change does not attempt any further assumptions, coercions or transformations.

References #465

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've run npm run cs-format on my branch to conform my code to prettier coding style
  • I'm adding a new feature
    • I've updated the playground with an example use of the feature

Modifies the implementation of `getSchemaType` so that when a property
has a type of `[<any>, 'null']`, then `<any>` will be passed. This
allows the library to render and validate the field correctly. For the
sake of simplicity, this change does not attempt any further
assumptions, coercions or transformations.

References rjsf-team#465
@wms
Copy link
Contributor Author

wms commented Mar 8, 2019

I wanted to land this PR in draft status before the end of the week in the hope that it'll get some eyes over the weekend. If this feature is desirable to the project maintainers, I'll add some tests and a Playground example when I return on Monday.

@wms
Copy link
Contributor Author

wms commented Mar 11, 2019

@epicfaace Forgive the direct tag - this is ready for review, but I can't get the PR out of draft status. Hopefully the controls will appear for project maintainers.

@epicfaace epicfaace self-assigned this Mar 11, 2019
@epicfaace epicfaace marked this pull request as ready for review March 11, 2019 16:17
docs/index.md Outdated Show resolved Hide resolved
@epicfaace
Copy link
Member

Looks good! Thanks for the PR. One question: how does the code actually know to put null as the value when you leave a field empty that has type: ["string", "null"] ? Is this coming from ajv?

Can you add a test in which it renders a form with a nullable field and checks to make sure that when the field is emptied, its value is null?

Co-Authored-By: warrenseymour <warren@fountainhead.tech>
@wms
Copy link
Contributor Author

wms commented Mar 11, 2019

@epicfaace This PR doesn't actually go that far; having looked the history of closed, unmerged PRs that attempted to implement UX changes or other additional handling of such nullable types, I intentionally decided against that. This allows end-users to handle such scenarios themselves, either through custom widget implementations or uiSchema adjustments.

To quote from the FAQ entry change:

However, the actual rendering and handling of the field is unchanged; you are free to handle this using an approach ('ui:defaultValue': null, for example) best-suited to your use case.

Should I expand on this in the FAQ and/or elsewhere to make this point more clear?

docs/index.md Outdated Show resolved Hide resolved
@epicfaace
Copy link
Member

@warrenseymour oh I see, thanks for the explanation. Just one small typo then and I think we'll be good to go! Thanks again!

I do think it might be nice to have some default handling of nullable types, but that can be added in the future as this change already goes pretty far.

Co-Authored-By: warrenseymour <warren@fountainhead.tech>
@wms
Copy link
Contributor Author

wms commented Mar 12, 2019

@epicfaace I completely agree. I've already started looking at how a nullable type can be rendered/handled in other layers of the library (perhaps in some opt-in manner, or a strictly 'bring-your-own-template' fashion), but that's another PR!

@epicfaace epicfaace merged commit d9bcf1a into rjsf-team:master Mar 12, 2019
@FriOne
Copy link

FriOne commented Mar 13, 2019

@epicfaace hi, could you bump this change into npm?

@epicfaace
Copy link
Member

I don't have access, @glasserc does

@FriOne
Copy link

FriOne commented Mar 15, 2019

Thought this solves validation problem when you want 3 types of values - any, undefined and null, but it is for a widget only :(

@wms wms deleted the allow-nullable-types branch March 15, 2019 09:06
@epicfaace
Copy link
Member

epicfaace commented Mar 15, 2019

@FriOne I don't think it's possible to set a value equal to undefined in json schema. Feel free to make another issue regarding this so we can discuss your use case further though.

@FriOne
Copy link

FriOne commented Mar 16, 2019

@epicfaace sorry, I meant that this change doesn't allow null in values. I can set string or undefined and validation will be passed but can't set null.

@epicfaace
Copy link
Member

@FriOne it should allow null in values. I've just updated the playground to the latest version; can you make an example on the playground and share it with me?

@FriOne
Copy link

FriOne commented Mar 18, 2019

@epicfaace the problem is on my side, my bad :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants