alexmojaki
commented
3 years ago I think the API needs some redesigning. The names test, check, and validate are easy to confuse, there's a strict copy of each one, and it's hard to add more options without creating a combinatorial explosion of methods. In particular I want to add options for https://github.com/gristlabs/ts-interface-checker/pull/4#issuecomment-808807995 and https://github.com/alexmojaki/ts-interface-checker/pull/1, and there may be more in the future such as an option to deal with #46 or #38.
I propose having one central method which users are encouraged to use where they can configure everything, something like this:
export class Checker {
// This is the central method, but we could create a new method with a new name instead
public validate(value: any, options: CheckOptions): IErrorDetail | null {
...
}
public check(value: any, options: CheckOptions): void {
this.validate(value, {...options, onError: OnError.throw});
}
public test(value: any, options: CheckOptions): boolean {
return this.validate(
value,
{...options, onError: OnError.return, numErrors: 0}
) == null;
}
...
}
interface CheckOptions {
extras: Extras
onError: OnError
/**
* Number of errors that can be nested directly
* under another error.
*
* Internally, the number of forks a DetailContext can hold.
*
* 0: Don't record anything.
* - Used for test() to just return a boolean
* - Could be used in `check()` if someone wants to raise an error
* but doesn't want a message.
* - Creates a NoopContext
* 1: Old default behaviour for check/validate
* > 1: Can nest several errors
* - May lead to a multiline error message when using OnError.throw
* - Requires checkRoot to be true
*/
numErrors: number
/**
* If true, wraps the top level error in another layer about the
* root type being checked. For example, this would change
* the error message from:
*
* value.extras is missing
*
* to:
*
* value is not a CheckOptions; value.extras is missing
*
* Similarly the returned IErrorDetail will have an extra top layer.
*
* This is required if numErrors > 1 so that multiple errors can be
* nested under the root IErrorDetail.
*
* Internally this wraps the type being checked in a TName first.
*/
checkRoot: boolean
}
/** What to do when extraneous values are found */
enum Extras {
/** Allow extra values */
ignore,
/** Formerly strict mode: do what onError says */
error,
/** Remove extra values in place */
delete,
}
/** What to do when an error is encountered */
enum OnError {
/** Throw a VError */
throw,
/** Return an object describing the errors */
return,
}Questions:
- Do you like this idea?
- What should the default values for options be? Keep behaviour the same? Default
numErrorsto 3? Require specifying certain options? - Deprecate some old methods? I suggest deprecating the
strictmethods, but leavingcheckandtestas is for the clean return types. - How to deal with conflicting options, e.g.
numErrors: 1, checkRoot: true, orcheck(value, {onError: OnError.throw})? Just overwrite them, or throw an error?
dsagal
This is an attempt to solve #3 and #16, allowing the result of
validateto have multiple errors in the samenestedarray so that users can solve several problems in their input data at once.At the moment this is a proof of concept, there's still quite a bit of work to do and decisions to be made, including whether to go forward with this at all.
Most types don't need to be able to report multiple errors so they haven't changed. Some types we could consider adding this to but I'm reluctant about:
So for now it's just TIface and TIntersection. TIface is a good example to show how this works.
First, this line is unchanged:
If it's not an object, there's nothing else to report, so we call
ctx.faildirectly and then return early. But from here on,failmust no longer be called directly onctx, at least until this checker returns. Failures will instead be stored in forks. These could be renamed to branches, children, subcontexts, etc. They should be used as follows:ctx.fork()ctxfor (potential) failures, whether it's in a deeper checker as inpropCheckers[i](v, fork)or directly as infork.fail(name, null, 1).return !ctx.failed()to check whether any failures were gathered along the way without triggering an early return.The NoopContext still does very little, only storing a boolean and returning itself from
fork(). But the extra operations have some inevitable overhead. I haven't done proper calculations yet but at a glance the benchmark (great thing to have!) seems to run something like 10-15% slower.The DetailContext keeps forks that have failures in them. The way it does this right now is not very clever and will probably be optimised. The number of failed forks it keeps is limited (right now to 3, but that will be made configurable) so that users don't get flooded with errors. Once it reaches that limit,
ctx.completeFork()returns false to indicate you should exit. But since you can now have a whole tree of contexts, the total number of forks and errors is unlimited.Some things that still need addressing:
What are your thoughts so far?