(core, testers) allow other additional types besides 'string' when testing for format

[jdwit]

I came across an issue where I have the following schema:

{
  "type": [ "string", "null"],
  "default": null,
  "format": "date"
}

As far as I know this is valid JSON schema, and is not really an edge case: a date field might be allowed a NULL value (maybe I am wrong here).

Currently the isDateControl and isDateTimeControl do not work here because they formatIs tests for schema.type === 'string'. This is replaced with hasType(schema, 'string') so the format is recognized correctly when other types are allowed (such as null) as well.

[sdirix]

Hi @jdwit,

Thank you for the PR ❤️ ! In general this make a lot of sense. However there are some considerations.

Changing the behavior of formatIs is a breaking change. Not every developer using it might be expecting null (or even completely arbitrary) values in their renderer, potentially leading to runtime crashes. As we're shortly before the 3.0 release this is a good opportunity to change this, but we need to add an entry to the migration guide. Please add this to the PR.

In then same vain we need to test all our renderers which use formatIs (React Material, React Vanilla, Angular Material, Vue Vanilla, Vue Vuetify) whether they can handle arbitrary value types. Can you help with this endeavor?

[coveralls]

Coverage remained the same at 84.29% when pulling f941910 on jdwit:core-allow-additional-types-when-testing-format into 23e1a55 on eclipsesource:master.

[sdirix]

Thinking about this some more: We should probably adapt formatIs to ONLY allow for 'string', ['string'] and ['string', 'null']. I'm not even sure what to render in cases where the type is something else.

Additionally when the type array is ['string', 'null'] the renderer should recognize that and place null instead of undefined in the data when they are cleared.

What do you think? Is this the same use case you have?

[jdwit]

Hi @sdirix , I can help providing the test coverage and adding an entry to the migration guide. Interesting point you make on the limitation of only string and null, I would think that the JSON Schema spec should be leading in this. I can imagine a case where a field can contain a string with of format of date OR a number with minimum of 10, format exclusively belongs to string type and minimum to number type. The schema validator thereby selects the relevant restrictions for the value. I am not an expert on JSON Schema but I think the above is described here: https://cswr.github.io/JsonSchema/spec/multiple_types

However this is not really compatible with the concept of selecting a renderer (which must represents a single type/format) 😄 . Considering this your suggestion of only allowing null and string makes more sense.

The thing you mentioned about null being the value when field is cleared, if null is in the collection of allowed types is exactly my use case, I am going to add this to must custom bindings set, thanks. I am not sure if this should be part of the JSONForms core however.

[sdirix]

Hi @jdwit, I'm fine with merging this as is in case the format renderers can handle problems relatively gracefully. The null handling should probably be done in a follow up later on.

Did you have a chance to test them?