fix: concat of required() for array/string

[vonagam]

Number 1 from #454

Removed redefinition of required() from array and string, instead added _isFilled(value) method for subtypes to override. Redefinition of required() causes problems with concat:

mixed().required().concat(array()).isValidSync([]); // now false, not true

[jquense]

I like this approach (along with the consolidated skip absent option) because it gets us halfway to solving another issue around allowing empty strings as empty values to be configurable. However I don't think this fixes the real problem here, just avoids it. Extending and overriding schema is supported and concat should still work in those cases.

[vonagam]

1. While making original PR, i also added required({ fill: false }) (in a sense that filling is not required and empty value is ok). But then removed it, since i understood you stance on required() and thought that you may not favour such idea. What do you think is better required({ fill: false }) or defined()?

2. Yes, having ability to override tests in subtypes is cool feature to have, but a) it is currently not present, b) it is complex things to do, c) it is not documented (in readme extending mentioned only in a sense of adding new transforms and new methods, nothing about overriding), d) in codebase only array.required()/string.required() do override, and this override gives two bugs: mixed.required().concat(array) mentioned here and array.required().notRequired() mentioned in original PR. So i am not really agree with assessment that "don't think this fixes the real problem here, just avoids it". I think it fixes two real bugs, which come from using currently unsupported behaviour. (Plus describe() with override as currently, gives unneeded test entry, can be counted as half bug).

[jquense]

Schema are structured as classes explicitly to allow this sort of thing, the documentation on extending schema also actively includes overriding a base method, that it's not a test shouldn't matter. So yes it's definitely fixing the bug by avoiding the problem. I don't necessarily think that's a bad thing in this instance but i don't think it's worth trying to frame it differently.

[vonagam]

the documentation on extending schema also actively includes overriding a base method

All examples are about adding new format() test method. Or are you talking about overriding _typeCheck() in last example? This is not a test method, it is supported to be overridden like all methods which start with underscore, _isFilled supports it too.

I see no easy way to support overriding any method. We have around four categories of methods for this:

1. Test addition. (like required())
2. Change of variable. (like nullable())
3. Transform (and optionally test) addition. (like compact())
4. Helpers/utilities which do not change anything. (all other stuff)

So, category number 4 is supported, but first three are not. One way to support them is to add invocation tracking for those methods and in concat instead of merging objects, you replay records of invocations in mutable mode (kinda like you do with test()). (Oh, i just realised that you do not use mutable mode in concat... will open a PR.)

It still will require writing such system, it will not be simple as just extending class as you will need to register such methods and concat will suffer in performance... I am not really sure that this feature worth the cost.

[jquense]

sorry I'm not actually convinced the behavior you're describing above is a bug.

mixed().required().concat(array().required()).isValid([]) // false

mixed().required().concat(array())).isValid([]) // true

mixed().required().concat(array())).isValid(undefined) // false

The above is what I'd expect when merging instances of different types together, you aren't going to get the more restrictive check if the more restrictive type doesn't contain that check. I wouldn't expect the mixed.required to suddenly understand that the value should also now have a length. I realize that that's up for debate but i don't actually think this is surprising behavior.

[vonagam]

Some thoughts and points for single required():

"you aren't going to get the more restrictive check" - only other check which is type dependent is type check itself, and it gets more restrictive.

required() is actually notEmpty()/filled(): this way it makes sense why an empty array or string, or null for a nullable type fail their validations, because they are considered empty for their type. So if we use such alias, is that really expected mixed().notEmpty().concat(array())).isValid([])?

required() (notEmpty()) needs type knowledge (like type check) to understand what is considered as an empty value (thats why array and string override required() right now). (And proposed way is the same as type check does - through override of a function which is accessed through this.schema in a test)

If mixed.required() is a valid separate test for an array, why can't i add it to array() instance directly (not through mixed().required().concat() hack)?

Handling double presence of required in describe() output, i assume that what caused a problem number 4 in original PR.

Overall, i think having multiply variants of required() will lead to more code, more complex code and some confusion (missing notRequred() override or describe() problem are examples).

(I think if it was named notEmpty or filled there would have been less issues in general, maybe filled() = required().notEmpty(), where required() just means not undefined. That would have been ideal...)