tdegrunt / jsonschema

JSON Schema validation
Other
1.83k stars 262 forks source link

fix type for Schema.required #293

Closed jaredh159 closed 4 years ago

jaredh159 commented 4 years ago

the required property can be either an array of strings, or a boolean, as per the example docs: https://github.com/tdegrunt/jsonschema/blob/9cb2cf847a33abb76b694c6ed4d8d12ef2037201/examples/all.js#L116-L147

Fixes #291.

jaredh159 commented 4 years ago

@tdegrunt Hey any chance you could look at and merge this PR? Would be really great to have accurate typings. Really appreciate all your work on this library, it's great!

awwright commented 4 years ago

@jaredh159 The behavior right now is it only shows array of strings, right?

The boolean form is deprecated, is there a good way to indicate that?

jaredh159 commented 4 years ago

@awwright the current type file only permits an array of strings, whereas in reality, boolean is also allowed.

I have not seen anywhere indicating that the boolean form is deprecated. Can you point me to that? I far prefer the boolean format as it allows me to keep the information that the property is required co-located with the rest of the info about that property, rather than at a distance.

Plus, even if it is deprecated, the type file should reflect the real functionality and not suggested usage or best practices.

awwright commented 4 years ago

I call it "deprecated" in the sense that it's no longer a standard form of JSON Schema. Though I don't have any plans to remove it from this library.

There's a few motivations to change it to a set of property names. It helps make schemas more composable and modular. If you reference a schema with {required: true}, it might have effects you don't want.

JSON Schema has considered a "requiredProperties" keyword (whereas "properties" are all optional). This might be the compromise that's best. See:

How would something like that work for you?

jaredh159 commented 4 years ago

@awwright thanks for the feedback. i hadn't really thought about composability of schema's, that makes sense. i perused the two issues you mentioned, they seem to be addressing concerns that aren't mine exactly -- i have a relatively small surface area of schemas that i control myself, so I'm more concerned with the developer ergonomics and readability of setting required: true, right next to the other data about the properties, instead of at a spot that can be fairly far away.

since you don't have any plans to remove the functionality from this library, is there still a reason not to fix the type files? like i mentioned earlier, typescript typedefs should always reflect what the program actually does and accepts. for instance, if this package were written in typescript, the type declaration files would be generated automatically, and the boolean value would be permitted, because the code itself permits it. so having the current type definitions not specify boolean seems like an error, even though I understand that you would rather encourage most users to leverage the array of strings variation.

when declaring types in typescript, you can put comments above them which will get picked up by most editors and ide's -- so a comment in the type def file could theoretically communicate the "deprecated" nature of the boolean format. however, in this case we would just be putting a comment on a sub-attribute of a type, and I don't think editors or IDEs will read the comment, so I don't think that buys us anything in this case.

jaredh159 commented 4 years ago

@awwright -- wondering where we're at with this PR -- whenever you have a chance. Thanks!

awwright commented 4 years ago

I'll try to take a look at this tomorrow!

jaredh159 commented 4 years ago

@awwright any chance you could take a look at this and merge? Thanks! 😄

jaredh159 commented 4 years ago

@awwright sorry to keep bugging you, but thought i'd try one more time before i started just depending on my fork... Are you OK with merging this and releasing? Thanks so much!

awwright commented 4 years ago

It's alright, I did say I'd get to this. I'm terribly sorry, I'll take another stab at this within the day.

Remember: This behavior is deprecated (more specifically, it's gone from JSON Schema), so no guarantees this behavior will be here in the future. There'll be a major version bump if it does, of course.

jaredh159 commented 4 years ago

Thank you! 🙏

awwright commented 4 years ago

@jaredh159 Check v1.2.6

jaredh159 commented 4 years ago

@awwright v1.2.6 works like a charm. 👍