Minor tweaks regarding required: true (encourage v4-style required)

[jonasfj]

• Remove usage from example in README.md, encourage v4-style required: ['prop', ...] instead.
• Throw error if undefined is given as input (it's invalid JSON, hence, invalid input).
• Support for disabling v3-style required: true.

---

I've added the option requiredV3 which defaults to true, to preserve backward compatibility.
It's probably best to disable by default, because it's an implementation specific feature, like filter.
(Note, there might be a better name for the option than requiredV3, suggestions are welcome).

Long-term this is probably something that should be disabled by default, you can probably roll that out with a major version upgrade.

---

Breaking change, some of the test cases used undefined as input. This is not a valid JSON value, so it's IMO acceptable that is-my-json-valid exhibits undefined-behaviour. I would surely expect weird things to happen if I tried to run validate on a Buffer object or some other non-JSON value :)

The reason I'm phrasing like this is that, before code like:

validate = validator({type: 'array'});
validate(undefined); // No error before

Would not return a validation error. But if the schema had been {type: 'array', required: true} it would have returned an error. required: true is not valid v4 JSON schema, and should not be necessary, however, strictly speaking there is nothing wrong with this behaviour because undefined is invalid input -- so undefined/implementation-specific behavior is fine :)

Implementation-specific behaviour is, however, a dangerous footgun. Powerful, but best hidden behind option flags, like filter.

I suggest we throw a runtime error. I don't think we should return a validation error, because undefined is not a JSON value, hence, it's an input error, not a validation error (similar to someone providing a Buffer object as input). It's probably not necessary to guard against all possible input errors, like circular object references etc. but undefined is probably a common input error that people easily end up relying on.

[chrahunt]

Good changes, I think these take the module in the right direction. Do you mind making a separate PR to include:

1. README changes (feel free to also change the tests to use the v4 format), and
2. Remove reliance of tests on undefined?

I can merge that in immediately.

@mafintosh Do you think that changing the behavior on undefined would be a good candidate for v3? I can create a separate issue and set the milestone if that is the case.

@jonasfj, concerning the v3-style required, do you think that it would be better to remove this altogether in the next major release? Assuming its impact on the undefined behavior you mentioned is handled by implementing your suggestion, and tools like this can convert v3 to v4 schemas directly, is there any benefit to keeping it?

[jonasfj]

I'll take a look at putting (1) and (2) in a different PR. Tomorrow, or Thursday maybe...

[jonasfj]

PR #73 contains (1) and (2), I still advice we merge the whole thing... So users can actively disable v3-style required which you probably want to disable in the next major release.

@chrahunt

concerning the v3-style required, do you think that it would be better to remove this altogether in the next major release?

I would certain disable it by default at the very least. But removing the support completely makes a lot of sense too, with modules like json-schema-compatibility there is really no benefit to keeping it. Especially since the cost of conversion is paid at compile-time/startup and not at run-time.

[LinusU]

I would love for this to get merged. I was just bitten by the validate(undefined) -> true in what I think is a quite normal pattern.

I was validating the request body in express using validate(req.body). Unfortunately I had forgot to include the json-parsing middleware on that route, and thus req.body was undefined. I would have expected the validation to not go thru, but it did, and it later ended in an error when actually trying to use the values.

Personally, I would release a new major release and drop the support of any JSON schema version below 4. It's not hard to upgrade to a newer version and I don't think that anyone will miss the old required syntax too much.

@mafintosh Do you have any updates on this, how do you want to proceed?