Skip to content

Conversation

@P0lip
Copy link
Contributor

@P0lip P0lip commented May 8, 2019

No description provided.

@P0lip P0lip requested review from casserni and lottamus May 8, 2019 21:32

function getType(node: JSONSchema4) {
if (Array.isArray(node.type)) {
return node.type.length === 1 ? node.type[0] : node.type;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We return a string if there's only one, but an array if theres none or more than one? Is that correct? Just making sure

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We return a string if there's only one, but an array if theres none or more than one? Is that correct? Just making sure

Well, I'm not sure how to handle such a scenario. For instance, there is no way we can display both array of items and object with properties.
Our UI is just too simple.

We would need to have something more or less similar to http://jlblcc.github.io/json-schema-viewer/ to handle such a case.

@marbemac what your thought on that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, we should be handling not, which we don't at the moment.
Any suggestions on the UI?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is no way we can display both array of items and object with properties

Is this what you mean?

image

Or something like oneOf?

image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is not?

We don't have to handle array and object in same property. We can suggest people use oneOf if they need that, although its kind of crazy and definitely an edge case.

Copy link
Contributor Author

@P0lip P0lip May 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lottamus yeah, pretty much that

{
  "type": ["array", "object"],
  "properties": {
    "bar": { 
      "type": "string" 
    }
  },
  "items": [
     {
       "type": "number"
     },
     {
       "type": "object",
       "properties": {
          "foo": { 
            "type": "number" 
          }
        }
     }
  ]
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, we can skip not for now - I haven't seen it used in the wild by our customers yet, and would need to support it in JSE as well. A good addition for later, but not high priority right now.

@lottamus
Copy link
Contributor

lottamus commented May 9, 2019

Do we have this fix on the v2 branch as well?

@P0lip P0lip merged commit c50ec21 into master May 15, 2019
@stoplight-bot
Copy link
Collaborator

🎉 This PR is included in version 1.3.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@P0lip P0lip deleted the fix/array-of-objects-with-properties branch May 15, 2019 19:25
@P0lip P0lip mentioned this pull request May 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants