Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Custom string formats #1186

Merged
merged 5 commits into from
Apr 5, 2019
Merged

Custom string formats #1186

merged 5 commits into from
Apr 5, 2019

Conversation

pushred
Copy link
Contributor

@pushred pushred commented Feb 20, 2019

Reasons for making this change

This is based off #1130, following it as a pattern for passing through additional configuration to AJV.

A prior PR, #889, attempted to add this. However I disagree with the guidance there to use custom widgets. That approach does not allow custom defined formats to be validated outside of the widget. Sure, validation logic can be implemented within and only emit values onChange on valid input. But should a parent form, custom validation functions, etc. not have awareness of validation status for all fields?

#889 opted to ignore custom formats altogether which I agree is ill-advised. AJV implements an addFormat method however that can associate validation patterns (and more) with named formats. This suppresses AJV warnings about these formats and validates data. Using formats also allows AJV to be used externally for validation in other contexts, e.g. on a backend.

Granted this is all similar to schema definitions but the key benefit is automatic assignment of custom widgets without requiring these to be specified as uiSchema wherever a definition is referenced. AJV's own custom validation functions, etc. may also be useful for advanced usage.

Lastly, json-schema-org/json-schema-spec#563 documents broader need for this capability in JSON Schema. This library itself has added color and data-url. Meta-schemas are not a viable solution as the above issue notes. A separate vocabulary definition file format may be specified in draft-08 but in the meantime being able to register such custom vocabularies with AJV would fill the need and give users of this library a solution until draft-08 arrives.

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

⚠️ I am opening this early, before #1130 is merged, to demonstrate this separate (but somewhat related) use case and see whether this is an acceptable solution before doing anything further.

@pushred
Copy link
Contributor Author

pushred commented Feb 22, 2019

Something of a blocker I ran into trying to add this to the playground — there's not a way currently to define config that is not serializable to JSON. For additionalFormats I can specify validation patterns as a string, which AJV converts to RegEx.

But custom widgets must also be registered in order to use these formats in react-jsonschema-form. In my usage of custom formats I typically map these patterns or apply other configuration to instances of a TextWidget of my own implementation. I could add some examples of this to app.js as is done for the GeoPosition / custom component example. But the playground isn't really representative of real world use with this approach.

I think a broader overhaul of the playground is warranted, using something like react-live to allow some access to react-jsonschema-form. schema and uiSchema could still be editable externally and validated as JSON. Happy to take this on, I've done similar work for an internal UI library. I also have a design background and could improve it's general usability and utility.

@epicfaace
Copy link
Member

Looks good, although of course we will need to wait for #1130 to be merged first before merging this one.

@epicfaace
Copy link
Member

@pushred -- since #1130 has been merged with some changes, do you mind making your changes again off of master and updating this PR to reflect that?

@epicfaace
Copy link
Member

@pushred just bumping this!

pushred added 3 commits March 15, 2019 20:04
Follow AJV extension pattern established for meta schemas and passthrough to AJV's addFormat method
@pushred
Copy link
Contributor Author

pushred commented Mar 16, 2019

@epicfaace thanks for the diligence!

I've rebuilt the branch off the latest and adapted my changes to use the new AJV instance. I hoisted that call outside of the meta schema handling so that both types of additions are applied to it. I considered adding a test to validate both function concurrently but wasn't sure about adding it to the meta schemas describe block and didn't want to duplicate the setup there. Let me know if you think that's valuable — I think including it in the meta schemas block would be best if so.

I also followed the lead of meta schemas and now accept new formats on prop updates.

Lastly, I renamed the prop and some code to customFormats vs. copying the additional prefix. This is shorter and consistent with AJV's docs.

@epicfaace
Copy link
Member

@pushred looks good, thanks for the good work!

I don't think it's necessary to make a test to check for meta schemas and custom formats concurrently. However, could you please add a test in this style? (something like "should update allowed custom formats when customFormat is changed"): https://github.com/mozilla-services/react-jsonschema-form/blob/92233e40e75854b50754a836a9b999c11eff75e7/test/Form_test.js#L1983

pushred and others added 2 commits March 22, 2019 11:54
@pushred
Copy link
Contributor Author

pushred commented Mar 22, 2019

@epicfaace good call on the prop update test, that caught a big oversight! Custom formats would not have been applied to validations on prop updates. Added the test and a fix for that.

@epicfaace
Copy link
Member

Thanks!

@epicfaace epicfaace merged commit e409be5 into rjsf-team:master Apr 5, 2019
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants