Add support for anyOf #417

stathismor · 2016-11-23T18:06:56Z

Reasons for making this change

This change adds support for anyOf. The logic is built in the ArrayField component and handles all the various supported types (number, string, array etc).

Areas that need improvement:

The layout is maybe not the prettiest.
UISchema is not supported for anyOf. The challenge here, as far as I understand it is that UISchema needs specific types defined in the JSONSchema, whereas with anyOf, the type is determined on the fly.
Array with anyOf, cannot also have fixed items at the moment.
anyOf item of type object is not supported. null is also not supported, but I saw there is a pull request (Initial implementation of nullable type #329) for that.

Checklist

I'm updating documentation
- I've checked the rendering of the Markdown text I've added
- If I'm adding a new section, I've updated the Table of Content
I'm adding or updating code
- I've added and/or updated tests
- I've updated docs if needed
I'm adding a new feature
- I've updated the playground with an example use of the feature

…onschema-form

cyxou · 2017-01-11T08:12:15Z

Why it hasn't been merged yet?

stathismor · 2017-01-22T16:18:43Z

@cyxou I am wondering this myself... I am happy to re-work on it, if there are any comments, and update the branch (there's quite a few conflicts as ArrayField has been updated since my PR).

inostia · 2017-01-26T21:30:07Z

+1, my project requires this and is on hold until this gets merged.

Natim · 2017-01-27T09:19:51Z

+1, my project requires this and is on hold until this gets merged.

Don't hesitate to use this branch in your project in the meantime.

kkarimi · 2017-01-27T10:34:23Z

+1 for merging

Natim · 2017-01-27T10:41:38Z

I guess the most important missing feature is the support of uiSchema. The anyOf feature is important to support but this PR doesn't support it yet at the standard we want for that lib.

We need at least to improve the UX of using anyOf as well as supporting the uiSchema.

Merging is easy but supporting an incomplete implementation of the spec in the lib is a path we are not prepared to take.

However this work is a great start and we are highly interesting in this, feel free to keep iterating and improving this Pull-Request and we will merge it as soon as we feel comfortable with maintaining it on the long term.

stathismor · 2017-01-27T11:04:38Z

Thanks for the detailed response @Natim! I wasn't aware what was wrong with it, but after your explanation, it makes sense. I remember updating the uiSchema was quite challenging, as it does not support dynamically added types... But now that I see there's some interest in this PR, I will keep working on it, first by updating it with master, then trying to add support for uiSchema

exchgr · 2017-02-09T21:36:55Z

src/components/fields/ArrayField.js

  };

+  anyOfOptions(anyOfItems) {
+    return anyOfItems.map(item => ({value: item.type, label: item.type}));


Should it be label: item.title?

Good spot, thanks! I think it should, yes. Maybe item.title || item.type, in case it's not defined...

Sounds good to me!

exchgr · 2017-02-09T22:19:16Z

Will this be able to support inheritance? Suppose I have an array of widgets. A widget can be of several types: Button, Slider, and Select. Button and Slider have the same attributes, but Select allows the user to define what's in its list.

The issue I'm running into is that they all need to be "object"s, and having multiple items in an anyOf array with the same type causes it to just select the first item with that type.

Here's an example of what I'm trying to do:

items: {
  anyOf: [
    {
      type: "object",
      title: "Button",

      properties: {
        name: {
          type: "string",
          title: "Name"
        }
      }
    },

    {
      type: "object",
      title: "Slider",

      properties: {
        name: {
          type: "string",
          title: "Name"
        }
      }
    },

    {
      type: "object",
      title: "Select",

      properties: {
        name: {
          type: "string",
          title: "Name",
        },

        fields: {
          "$ref": "#/definitions/select_fields"
        }
      }
    }
  ]
}

stathismor · 2017-02-10T00:41:51Z

@exchgr I see what you're saying. This was deliberate as I did not have a use case for multiple items with same type. I think that part of code responsible is this line, that tries to find the selected item in the anyOf schema.

It could be easily changed so that title is preferred instead type. But title is not an obligatory property and is not required by object. And it would break for cases where an anyOf schema has multiple types with the same title. I think there is a simple custom change you can make if you want. If the SelectWidget uses the title as the identifier instead of the type, then it should just work...

I guess the problem lies in the fact that there is no unique identifier between anyOf items. Ideally I think it should not reference the items by type, but also by the order in the schema. Or maybe, now that I think about it, it could be just the order... Thanks for the comment, I will check whether order can be used, next time I have a look at it!

exchgr · 2017-02-13T21:41:27Z

@jManji Thanks for the quick response. Going by the array index may solve the issue. I was also thinking that it'd be useful to have a way for the dropdown to influence an attribute that indicates the type on the backend.

Building on my example, suppose we have a few classes on the backend (in this case, Rails). Here's an example with polymorphic associations emulating inheritance across multiple tables (aka Multiple Table Inheritance):

class DeviceType < ApplicationRecord
  has_many :controls
  # we're also able to work with each control's "controllable" association directly, through some methods not defined in this example
end

class Control < ApplicationRecord
  belongs_to :device_type
  belongs_to :controllable, polymorphic: true
end

class Button < ApplicationRecord
  has_one :control, as: :controllable
end

class Slider < ApplicationRecord
  has_one :control, as: :controllable
end

class Select < ApplicationRecord
  has_one :control, as: :controllable
end

Then in the JSON schema:

items: {
  anyOf: [
    {
      type: "object",
      title: "Button",

      properties: {
        name: {
          type: "string",
          title: "Name"
        },

        controllable_type: {
          enum: ["Button"]
        }
      }
    },

    {
      type: "object",
      title: "Slider",

      properties: {
        name: {
          type: "string",
          title: "Name"
        },

        controllable_type: {
          enum: ["Slider"]
        }
      }
    },

    {
      type: "object",
      title: "Select",

      properties: {
        name: {
          type: "string",
          title: "Name",
        },

        controllable_type: {
          enum: ["Select"]
        }

        fields: {
          "$ref": "#/definitions/select_fields"
        }
      }
    }
  ]
}

Since I actually need this for something I'm working on, I'm going to take a stab at it today.

glasserc

Impressive!

Sorry for the delay in reviewing this -- @n1k0 has been busy on other projects and I'm only just coming up to speed on this project. Here are my thoughts on this approach.

How come the logic for this is part of the ArrayField class? Just to simplify, I guess? I think we should come up with a clear understanding of what cases we want to cover, and I don't think this includes all of them. As an example, this seems like something we'd want to support:

{
  "type": "object",
  "anyOf": [{
    "properties": {"foo": {"type": "number"}},
    "additionalProperties": false
  }, {
    "properties": {"bar": {"type": "string"}},
    "additionalProperties": false
  }],
}

[i.e. an object with two possible cases.]

Here's another one:

{
  "type": "object",
  "properties": {
    "userId": {
      "anyOf": [
        {"type": "number"},
        {"type": "string"}
      ]
    }
  }
}

[i.e. an object with a field that has two possible types; this particular example could be implemented using "type", but more complicated examples using objects can be found in the wild, per @exchgr's comment]

I'm not sure about this:

{
  "type": "object",
  "properties": {"version": {"type": "string"}},
  "anyOf": [{
    "properties": {"username": {"type": "string"}}
  }, {
    "properties": {"email": {"type": "string"}}
  }]
}

[i.e. an object with one mandatory property and two possible cases]

I'd say we should not bother even trying to support something like:

{
  "type": "object",
  "anyOf": [
    {"type": "integer"},
    {"type": "string"}
  ]
}

I'm not averse to accepting a PR that only supports some of these cases, of course, so long as it's maintainable and it supports implementing the other cases. I'm not sure this PR does, since it puts the logic in arrays rather than in items, where they would be both more powerful and I think simpler.

I also found the UI a little clunky -- rather than having a "select" to choose what kind of object it is, why not use something like Bootstrap's "radio" buttons functionality?

Also, why did you move items from arrayField.state.items to arrayField.state.formData.items?

glasserc · 2017-02-28T19:15:09Z

playground/samples/index.js

@@ -30,4 +31,5 @@ export const samples = {
  Files: files,
  Single: single,
  "Custom Array": customArray,
+  "Any of": anyOf


At first this bothered me, but I see our capitalization isn't consistent -- Custom Array vs. Date & time.

In any case (😉), leaving "of" as lowercase is correct, even when you're using title case. http://grammar.yourdictionary.com/capitalization/rules-for-capitalization-in-titles.html

glasserc · 2017-02-28T19:34:17Z

test/ArrayField_test.js

+    it("should render an add button", () => {
+      expect(node.querySelector(".array-item-add button"))
+        .not.eql(null);
+    });


Is this test really necessary? If there were no add button, the beforeEach would fail, wouldn't it?

glasserc · 2017-02-28T19:43:43Z

test/ArrayField_test.js

+    it("should render a select along with the input field", () => {
+      expect(node.querySelectorAll(".array-item"))
+        .to.have.length.of(1);
+    });


I think this test is misnamed, since it only checks for the array item, not for a select field.

glasserc · 2017-02-28T19:43:53Z

test/ArrayField_test.js

+    it("should render two select and input fields", () => {
+      Simulate.click(node.querySelector(".array-item-add button"));
+      expect(node.querySelectorAll(".array-item")).to.have.length.of(2);
+    });


Same here, no select fields.

glasserc · 2017-02-28T20:05:23Z

src/components/fields/ArrayField.js

+    const {definitions} = registry;
+    const anyOfItemsSchema = this.getAnyOfItemsSchema();
+    const newItems = items.slice();
+    const foundItem = anyOfItemsSchema.find((element) => element.type === value);


Does this work with things like $ref?

I've tried it & it doesn't support $ref

glasserc · 2017-02-28T20:23:47Z

src/components/fields/ArrayField.js

+      <div className="form-group" style={{width: 120}}>
+        <SelectWidget
+          schema={{type: "integer", default: selectWidgetValue}}
+          id="select-widget-id"


The same ID shouldn't be used more than once in a DOM.

glasserc · 2017-02-28T20:34:47Z

src/components/fields/ArrayField.js

@@ -50,6 +50,7 @@ function DefaultArrayItem(props) {
    <div key={props.index} className={props.className}>

      <div className={props.hasToolbar ? "col-xs-9" : "col-xs-12"}>
+        {props.selectWidget}


It seems kind of strange for selectWidget to be part of the props, and not part of the template.

b1r3k · 2017-03-17T11:41:44Z

@glasserc Thanks for review. Argument for extending support for anyOf beyond array makes sense + it's very useful to me at this moment.

For example I would like to use this schema:

{
    "definitions": {
        "TPNode": {
            "title": "TPNode",
            "description": "TPNode",
            "type": "object",
            "properties": {
                "name": {
                    "type": "string"
                },
                "value": {
                    "anyOf": [
                        { "type": "string"},
                        {
                            "type": "array",
                            "items": {
                                "$ref": "#/definitions/TPNode"
                            }
                        }
                    ]
                }
            }
        }
    },
    "$ref": "#/definitions/TPNode"
};

Here is how it should render

@jManji are you intending to work this PR according to @glasserc suggestions any time soon?

stathismor · 2017-03-20T22:23:36Z

Sorry for late reply guys. I've been quite busy lately, and didn't have time to properly look at this. @b1r3k yes, I would very much like to rework on this, when I have some time, based on @glasserc's suggestions. And there's a couple of things that came up from previous conversations here. I would say I should have something in the next couple of weeks.

@glasserc, thanks a lot for the detailed CR, appreciated! Just trying to answer some of your questions without touching the code yet:

How come the logic for this is part of the ArrayField class? Just to simplify, I guess?

I think that was because I wanted to make use of the ArrayField functionality that is there already, like adding multiple properties, all the UI stuff that comes with it, etc. anyOf looks and behaves a bit like an array, so I thought I could fit it in there (like for instance, how additionalItems feature does). Maybe a new component type makes more sense? That seemed much more work I guess.

I also found the UI a little clunky -- rather than having a "select" to choose what kind of object it is, why not use something like Bootstrap's "radio" buttons functionality?

You think radio is better for this case? How would you render lots of options? Will it not occupy too much space and make the form look a bit inconsistent? Not sure, but that's and easy change, if that's what we want.

Also, why did you move items from arrayField.state.items to arrayField.state.formData.items?

That's a good question... I must have had a reason, I will let you know on my next commit 😄. If I see no reason, I will just move it back to state.items

exchgr · 2017-03-21T14:44:42Z

Perhaps if this supports uiSchema, we could let the developer decide whether to use radios or selects.

glasserc · 2017-03-21T19:15:34Z

I like @exchgr's suggestion to support uischema. As a default, I'd propose this heuristic: if the anyOf options are only two or three, I think radio buttons are more convenient (just click the one you want); if they are four or more, a select is probably more respectful of screen real estate.

Maybe a new component type makes more sense?

Could we maybe put it on SchemaField? That was my first instinct (coming from a classical OO background), since any schema could potentially be anyOf. On the other hand, that might make SchemaField too complicated. Maybe @n1k0 has an opinion?

n1k0 · 2017-03-22T09:23:34Z

I also like the idea of using uiSchema to configure the type selector a lot, though I fear the added complexity. Personally, I'd probably iterate here. Good support for anyOf is already hard to get right :)

Otherwise I concur with @glasserc, SchemaField feels like the natural place to implement such support.

Side note, @jManji could you please try to install prettier v0.22.0 and run:

$ prettier --jsx-bracket-same-line --trailing-comma es5 '{playground,src,test}/**/*.js' --write

on your current branch, then try to merge/rebase it with latest master? I also fear we're gonna eventually diffing and conflicting a lot if we postpone this operation too much. Thanks!

stathismor · 2017-03-22T10:41:18Z

The challenge I see with uiSchema is that it is keyed by the property's key. You cannot just say, "I want anyOf items to be textarea" for instance. You have to define which anyOf item you refer to, and since we potentially want (currently this branch does not support it) multiple anyOf items of the same type, the uiSchema needs to probably reference the order/index of the anyOf item (?). I will definitely leave the uiSchema out of the scope of the next iteration. If we want, we can start by figuring out how a uiSchema for anyOf would look like.

@glasserc, @n1k0 Regarding moving code to SchemaField, it's been some time since I looked at the code, but currently this also feels challenging/weird to me. It will definitely make SchemaField more complicated as you said, and I think, it will need to have some kind of support for dynamically added widgets, like ArrayField has. But I am sure you know the codebase much better than me, so I will look into how this can be achieved.

Side note, @jManji could you please try to install prettier v0.22.0 and run...

Yes, will do!

... I also fear we're gonna eventually diffing and conflicting a lot if we postpone this operation too much.

Yes, you are right, I know... I will try to work on this asap, going through every comment mentioned here and in the code review.

b1r3k · 2017-06-08T16:02:18Z

@jManji any news regarding this PR? I'm willing to take it further if you do not have time. In fact I've forked your solution and fixed few mistakes in order to use it in our internal tooling but it has meant to be workaround and the further we go with development the more crippled we are due to divergence from origin/master

stathismor · 2017-06-09T11:15:01Z

@b1r3k I am very sorry but unfortunately I don't plan on working on it again, any time soon. I needed that feature for my work, but now we have shifted priorities. Please feel free to work on it!

I haven't checked in detail, but maybe #581 solves the same problem in a different way?

danrot · 2017-07-27T09:23:26Z

I have checked #581, but it looks to me like this is only built to be used in dropdown and selects. From what I've seen it doesn't allow you to build more complex objects than simple strings.

@b1r3k Have you continued working on it? Anything worth sharing? We would also desperately need a feature like this, although my use case would probably be a oneOf, although anyOf should also be good enough :-)

bchansell · 2017-11-28T15:46:41Z

Is there any way to install this through npm as a node_module?

epicfaace · 2019-01-19T14:13:36Z

Closing this as this functionality has been implemented in #1118.

Stathis added 9 commits November 21, 2016 11:05

Got something working

84aca60

Added tests

4d11758

Added anyOf file

cc2b5d6

Fix small issues and add tests

140c457

Any of and one of together

9b8249d

Remove oneOf

b2295d9

Merge branch 'master' of https://github.com/mozilla-services/react-js…

fbc25e7

…onschema-form

Add support for anyOf

2c0dc9f

Remove whitespace

00a96ac

This was referenced Nov 23, 2016

Support for joined schemas with anyOf, allOf, etc? #52

Closed

Prototype component demonstrating support for OneOf #302

Closed

Merge branch 'master' into feature/anyof-support

7d90cc6

stathismor closed this Jan 31, 2017

stathismor force-pushed the master branch from 3688b94 to dd71a85 Compare January 31, 2017 15:56

stathismor reopened this Jan 31, 2017

Remove debug statements

3460ec6

exchgr reviewed Feb 9, 2017

View reviewed changes

stathismor mentioned this pull request Feb 22, 2017

Remove setState in ObjectField and ArrayField #480

Merged

8 tasks

glasserc requested changes Feb 28, 2017

View reviewed changes

glasserc mentioned this pull request Mar 1, 2017

Initial implementation of nullable type #329

Closed

n1k0 mentioned this pull request Jul 25, 2017

Unsupported field schema "anyOf" #648

Closed

glasserc mentioned this pull request Sep 22, 2017

Easier Bootstrap Grid usage and basic BS4 compatibility #623

Closed

9 tasks

picodoth mentioned this pull request Jun 1, 2018

[WIP]feat: supporting anyof #940

Closed

10 tasks

LucianBuzzo mentioned this pull request Jan 3, 2019

Add preliminary support for the anyOf keyword #1118

Merged

9 tasks

epicfaace closed this Jan 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for anyOf #417

Add support for anyOf #417

stathismor commented Nov 23, 2016 •

edited

Loading

cyxou commented Jan 11, 2017

stathismor commented Jan 22, 2017

inostia commented Jan 26, 2017

Natim commented Jan 27, 2017

kkarimi commented Jan 27, 2017

Natim commented Jan 27, 2017

stathismor commented Jan 27, 2017

exchgr Feb 9, 2017

stathismor Feb 9, 2017

exchgr Feb 9, 2017

exchgr commented Feb 9, 2017

stathismor commented Feb 10, 2017 •

edited

Loading

exchgr commented Feb 13, 2017

glasserc left a comment

glasserc Feb 28, 2017

exchgr Mar 21, 2017

glasserc Feb 28, 2017

glasserc Feb 28, 2017

glasserc Feb 28, 2017

glasserc Feb 28, 2017

b1r3k Mar 20, 2017

glasserc Feb 28, 2017

glasserc Feb 28, 2017

b1r3k commented Mar 17, 2017

stathismor commented Mar 20, 2017

exchgr commented Mar 21, 2017

glasserc commented Mar 21, 2017

n1k0 commented Mar 22, 2017

stathismor commented Mar 22, 2017

b1r3k commented Jun 8, 2017

stathismor commented Jun 9, 2017

danrot commented Jul 27, 2017

bchansell commented Nov 28, 2017

epicfaace commented Jan 19, 2019

Add support for anyOf #417

Add support for anyOf #417

Conversation

stathismor commented Nov 23, 2016 • edited Loading

Reasons for making this change

Checklist

cyxou commented Jan 11, 2017

stathismor commented Jan 22, 2017

inostia commented Jan 26, 2017

Natim commented Jan 27, 2017

kkarimi commented Jan 27, 2017

Natim commented Jan 27, 2017

stathismor commented Jan 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

exchgr commented Feb 9, 2017

stathismor commented Feb 10, 2017 • edited Loading

exchgr commented Feb 13, 2017

glasserc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b1r3k commented Mar 17, 2017

stathismor commented Mar 20, 2017

exchgr commented Mar 21, 2017

glasserc commented Mar 21, 2017

n1k0 commented Mar 22, 2017

stathismor commented Mar 22, 2017

b1r3k commented Jun 8, 2017

stathismor commented Jun 9, 2017

danrot commented Jul 27, 2017

bchansell commented Nov 28, 2017

epicfaace commented Jan 19, 2019

stathismor commented Nov 23, 2016 •

edited

Loading

stathismor commented Feb 10, 2017 •

edited

Loading