is icon indicating copy to clipboard operation
is copied to clipboard

Enhance assert error messages

Open szmarczak opened this issue 4 years ago • 5 comments

is.assert.any:

https://runkit.com/szmarczak/5e53ec822c7e080014a4edf9

TypeError: Expected value which is `predicate returns truthy for any value`, received value of type `Array`.

IMO it should return:

Expected value of type `boolean` or `undefined`, got `null`

and if the check function is not an is function, simply:

Expected value of a custom type, got `null`

szmarczak avatar Feb 24 '20 15:02 szmarczak

// @joelpurra In case you have any thoughts on this.

sindresorhus avatar Feb 24 '20 16:02 sindresorhus

@sindresorhus Maybe we can save a Map<Function, string> where each of the keys is an is.XX function, and this is how we would get the method name? or perhaps this is something we can save on the method itself? maybe with a symbol or something 🤷🏻‍♂️

Like:

const IS_FUNCTION_NAME = Symbol('IS_FUNCTION_NAME');

is.boolean[IS_FUNCTION_NAME] = 'boolean';

gioragutt avatar Feb 24 '20 22:02 gioragutt

@szmarczak: yeah, that's not too helpful. The error message is hardcoded, like for other methods, and in addition the input value(s) always gets wrapped in an array because it's a rest parameter.

  • https://github.com/sindresorhus/is/blob/v2.1.0/source/index.ts#L444-L445
  • https://github.com/sindresorhus/is/blob/v2.1.0/source/index.ts#L391

@sindresorhus: since any and all are more dynamic than the other methods, the error message might as well also be more dynamic.

@gioragutt: a lookup map would work, and is easy to implement. The assertions are using a mixture of TypeName and AssertionTypeDescription, which is a decent separation but could perhaps be made cleaner/more consistent with a single map -- if there are no mismatching overlaps. Am not too keen on storing the name on the method itself.

  • https://github.com/sindresorhus/is/blob/v2.1.0/source/index.ts#L7
  • https://github.com/sindresorhus/is/blob/v2.1.0/source/index.ts#L406

In addition, the rest parameter means it'd have to handle multiple values. The (implicit) array might be a bit confusing, in particular for single values, so could do with or without the square brackets in the error message.

is.assert.any([is.boolean, is.undefined], null, "some string");
Expected value of type `boolean` or `undefined`, got [ `null`, `string` ]

joelpurra avatar Feb 25 '20 10:02 joelpurra

The error message that wraps the values (even if rest params) in brackets is clearer I think

gioragutt avatar Feb 26 '20 14:02 gioragutt

Just another vote for getting this Issue some more love. The error messages are pretty cryptic.

I admittedly haven't familiarized myself with the internals of is/got, but one thing that would really help would be knowing which property generated the error.

(For me and my usage of got, the problem was that I needed to use json instead of body. If it were more clear that the issue was with my usage of body (instead of a generic error message), I would have been much more likely to be able to fix the issue myself instead of having to come here for support)

My guess is that this issue is probably more relevant for a fix within the scope of got instead of is, but thought it might be worth bumping this issue as well.

rinogo avatar Jun 01 '22 22:06 rinogo

Just want to chime in on this issue – I am very much considering using this library but the thing stopping me right now is the lack of custom error messages. I have an existing pattern that allows for customized messages, like so:

// assertion functions need an explicit type declaration (?)
type AssertNonNull = <T = unknown>(
  val: T | null | undefined,
  message?: string,
) => asserts val is T;

export const assertNonNull: AssertDefined = <T = unknown>(
  val: T | null | undefined,
  message?: string,
) => {
  if (val === undefined || val === null) {
    throw new Error(
      message || `Expected non-null value: ${JSON.stringify(val)}`,
    );
  }
};

assertDefined(data.possiblyNull, '`data` has an invalid value for field `possiblyNull``);

I find this useful because oftentimes, error messages can be better-worded to suit the context in which they come up, as seen in the example.

Would something like this be feasible? I'm imagining it would look like this:

assert.string(data.stringOrNullProp, '`data` needs to have a non-null `stringOrNullProp` here');
// `data.stringOrNullProp` is now typed as a `string`.

ssalka avatar Mar 17 '23 18:03 ssalka