Injection of custom ajv instance via prop (+ removal of global ajv)

[sbusch]

Reasons for making this change

DO NOT MERGE YET.

A custom ajv instance can be created and injected to the Form as "ajv" prop. This allows further customizations of behavior.

Notes:

• createAjvInstance() from utils.js can be used to generate a preconfigured instance, which can be further customized (by adding custom keywords, ....).
• change allows multiple Forms with differing ajv configuration, e.g. different customFormats
• If no "ajv" prop is provided, the Form generated an own instance that is cached (memoized) depending on the ajv-related configuration props (additionalMetaSchemas and customFormats)
• Many utility functions needed to be modified to pass ajv instance through. I decided to add ajv as 1st parameter, because this is no optional argument

This feature allows to deprecate some features, for example Form features additionalMetaSchemas and customFormats could be achieved by an customized ajv, further simplifying react-jsonschema-form implementation.

I decided to release this now to get some early feedback. ajv injection and removal of global ajv is finished and tests are passing (see status, below).

I'll try to rebase this for some time on master, but since there are many changes quick feedback would be great...

Comments very appreciated!

Status:

• ajv injection and removal of global ajv finished
• all tests are passing (despite the three ones that are always failing, see my other PR Fix file widget tests #1285 for a fix)
• provide tests (work in progress, I started with a suboptimal testcase which needs to be reworked)
• provide playground example (work in progress, here I also started with a suboptimal example)
• update docs
• document changed signatures of exported functions (❓ are there API docs somewhere?)
• (maybe) deprecate props/features like additionalMetaSchemas and customFormats (deprecation marker in source, updated docs)
• Link to existing issues that could be solved by this PR (🆘 Some help here would be great)

Checklist

TODO

• I'm updating documentation
  • I've checked the rendering of the Markdown text I've added
• 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

[Mitgenosse]

Hi. Would it be possible with this to add the "verbose" keyword to the ajv instance so I can have more detailed error objects like the schema and parentSchema property? https://github.com/epoberezkin/ajv#error-objects

Or is this already possible but I am not aware of it...?

[sbusch]

@Gurkenschreck you can pass in any ajv instance, customized to your needs. react-jsonschema-form internally then uses this instance for its validations, but the validations results are processed internally first: take a look at the transformErrors prop, and the implementation on validateFormData and transformAjvErrors in src/validate.js.

Unfortunately, transformAjvErrors does some explicit conversions and the original error - including any additional info sent in verbose mode - gets lost.

IMO the original validation error should be added to each error, similar to #1140 but not just some keys.

For example:

 // put data in expected format
return {
  name: keyword,
  property,
  message,
  params, // specific to ajv
  stack: `${property} ${message}`.trim(),
+ _ajvError: e,
};

Currently no time for such a pull request.

[epicfaace]

@sbusch that's a good idea, can you make an issue for that?

[Mitgenosse]

Unfortunately, transformAjvErrors does some explicit conversions and the original error - including any additional info sent in verbose mode - gets lost.

Maybe if someone (maybe even me) changes it he can consider:
For i18n translations we have a key like "form.questionA.title" and it appears like

{
    "type": "string",
    "title": "form.questionA.title"
}

The error then says e.g. ".a.b[4].c is a required property". Which is not very nice for the user so in the transformErrors function we are looking in the schema for this property and try to get the "title" so we can render "Address 1: [error]" and kind of saturate the ajv error with the schema. If ajv appends the schema to the error when we set the "vebose" prop, that would be very nice.

[ankurkaushal]

Any updates on this PR?

Something like this would be really helpful for us since we have some forms where we have multiple fields that require comparisons. For example email, confirmEmail, password & confirmPassword are on the same form in couple of our pages.

Even though I could write multiple if logic in validate hook, but I would rather not. I would appreciate some thoughts on it. :)

[sbusch]

@ankurkaushal I wanted to wait for some feedback from maintainers if my approach is mergeable in principle before I proceed (rebasing it onto newer commits, ...) @epicfaace and other maintainers, whats your opinion?

@ankurkaushal You could try to build from my PR and try to implement your stuff using it, could be valuable feedback.

I'm sure that removal of global ajv singleton is a necessary step for the project, but when and how is the decision of the maintainers...

[ankurkaushal]

I am not familiar with the code myself (will probably have to sit down on a weekend & go through the code) so it would take me long time implement that myself & we are kind of in a time crunch so it makes things complicated too.

But yeah removal of global ajv singleton would go far in making the library more flexible.

[sbusch]

Ping also @edi9999

[epicfaace]

@sbusch I really like this approach, especially how it will simplify and allow for more flexibility than additionalMetaSchemas and customFormats allow.

Question -- why is createAjvInstance() needed when people can make their own ajv instances using ajv itself?

[edi9999]

It seems to be a great improvement to me !

[skortchmark9]

bump :)

[videni]

I also like this approach, wish this can be merged soon.

[sbusch]

I'm sorry but I am currently working on other stuff and have no time for the required changes, especially checking & modifying all upstream changes (for new parameter holding the ajv instance) will take some time.

If we had 100% test coverage, we could just merge and fix until all is green again, but I think that's not the case?!

And advice?

[epicfaace]

It would help to see which tests exactly are failing. Do you know why travis isn't running on this PR?

[videni]

what can I do to help? I really need this feature. 4 month passed, still no progress.

[epicfaace]

@videni would you like to take over this PR and get all the tests to pass? That would help a lot

[sbusch]

If TypeScript migration (#583, with #1446) is also a goal for v2, I propose to hold this PR off until that is finished:

Having typings in place - in conjunction with the tests - will help a lot when manually re-applying this PR (which is IMO necessary because of the conflicting nature of this PR vs. many upstream changes).

Additionally, the upgrade to React 16 (merged in #1408 ) will also simplify this PR, when my context workarounds can be removed. @epicfaace have you decided on the React version? (#1275)

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Please leave a comment if this is still an issue for you. Thank you.