Improve validation result error checking

[martijndeh]

One thing I'd like to enforce is to check for an error. If for some reason a developer forgets to check the error, the value of the Joi#validate(..) call happily returns a properly typed value object.

Before this change:

const mySchema = Joi.object().keys({ foo: Joi.string().required() }).required();
const result = Joi.validate(request.body, mySchema);
// result.value.foo is simply available which may cause issues if the validation failed but not checked

After this change:

const mySchema = Joi.object().keys({ foo: Joi.string().required() }).required();
const result = Joi.validate(request.body, mySchema);
// result.value.foo errors with `Object is possibly 'null'`.

This should be fixed by:

const mySchema = Joi.object().keys({ foo: Joi.string().required() }).required();
const result = Joi.validate(request.body, mySchema);

if (result.error) {
  // result.error available; result.value.foo undefined
} else {
  // result.value.foo available
}

One thing which doesn't work yet (but probably because of reasons tsc people might know):

const mySchema = Joi.object().keys({ foo: Joi.string().required() }).required();
const { error, value: { foo } } = Joi.validate(request.body, mySchema);

if (error) {
  // error; foo is string | undefined but should be undefined
} else {
  // foo is not properly infered but is string | undefined and should be string
}

The nice thing is that you don't have to add redundant check to see if props on the value are correct: the compiler is smart enough, if error is null, to infer props on value will be correct (except for the destructure use case).

It's seems, in case of an error, value is actually not null or undefined, but still some object so I made some last minute changes to address this. Also, I wasn't aware of the promise api and for the rest I don't know if this possibly breaks any use cases.

Let me know and feel free to make some changes to this merge request yourself. For example: I don't know what kind of types are possible in the value object.

[martijndeh]

Monday is fine. :)

Ah, I forgot to add the type, will do. I used Github's web interface because I wanted your thoughts on the change first before putting in more effort.

[hjkcai]

Hey @martijndeh

Sorry for the delayed reply. I have some questions about your patch.

1. Why ValidationResult is a union with 3 types? TypeScript cannot narrow a ValidationResult object into ValidationResultError or ValidationResultUnchecked by checking if ValidationResult.error is null.
2. Why do you preserve destructing? The target object might be validated against a string schema (e.g. Joi.string().required()). In this case the original string type will be turned into a meaningless type: an object with all String.prototype keys and all the values are undefined.

As above, I think the ValidationResult can be split into 2 types. That seems enough. What is your opinion?

type ValidationSucceed<T> = { error: null, value: T }
type ValidationFailed = { error: Joi.ValidationError, value: unknown }

[martijndeh]

When I look at my codebase, I often use the following pattern. My assumption is that this is used more often in user land.

const { error, value: { a, b } } = Joi.validate(..);

If value is unknown the destructuring doesn't work anymore and this change will require refactoring in user land if this pattern is used (of course this change will be breaking, but for the better, because you're probably checking value .

I think you're right with the unchecked. Maybe something like the below is better:

type ValidationSucceed<T> = { error: null; value: T };
type ValidationFailedOrUnchecked<T> = {
  error: Joi.ValidationError;
  value: T extends { [key: string]: any }
    // Make destructuring still work
    ? { [key in keyof T]?: T[key] }
    // Make sure it's possibly undefined when error not checked yet
    : T | undefined; };
type ValidationResult<T> = ValidationSucceed<T> | ValidationFailedOrUnchecked<T>;

The above also includes a fix for the following use case:

const result = Joi.validate(..) as ValidationResult<string>; // Joi.string().required()

// result.value is now string or undefined

if (result.error) {
 // It would be nice if result.value was 
} else {
  // result.value is string
}

I'm now seeing there is a case in TypeScript 3.5.2 where the type system can't infer the type correctly:

const { error, value } = Joi.validate(..) as ValidationResult<string>; // Joi.string().required()

// value is string | undefined

if (error) {
  // 
} else {
  // value should be string as there error is checked, but value it's string | undefined :/
}

I'll check the TypeScript roadmap to see if there is anything planned to improve this case.

[hjkcai]

I think the destructing issue will not be solved in a short time because the mechanism of type narrowing of TypeScript works only on one variable. But after destructing there are multiple variables. TypeScript has no way to know about the connection between them.

In another word, you SHOULD NOT destruct the ValidationResult in the current TypeScript version. Instead, destruct result.value.

const result = Joi.validate(..)
if (result.error) {
  // result.value is unknown
} else {
  const { foo, bar, ... } = result.value
}