Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/improved failure handling experience #331

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

zeeeeby
Copy link

@zeeeeby zeeeeby commented Sep 10, 2023

Hello!

I have 2 ideas (and ready implementations in this PR) to improve error message readability.

The first idea

I often encounter is overly detailed logging of complex structures in the "Expected" messages.

For example:
let's have this schema

const Obj = Record({
  id: Number,
  name: String,
  nested: Record({
    id2: Number,
    name2: String,
    nested2: Array(
      Record({
        nested3: Record({
          id3: String,
        }),
      }),
    ),
  }),
});

// and we will validate this object
const result = Obj.validate({
		id: 3,
		name: "test" 
})

The validation result will be very messy:

{
      nested: 'Expected { id2: number; name2: string; nested2: { nested3: { id3: string; }; }[]; }, but was missing'
}

It seems unnecessary to describe the expected type since it is already known in the runtypes schema description. Moreover, this description complicates error message readability.

Instead, I propose writing "Expected (array | tuple | object | dictionary) but was ..."

in this PR result will change to this

{
      nested: 'Expected object, but was missing'
}

It looks readable and doesn't lose its meaning.

The second idea

In messages like "Expected something but was (array | object)" it would be helpful to see exactly which entity was received.

I have decided to add a new field called extraDetails next to the details field, which will contain not only the error message but also the actual value (received field) and possibly the error code.

Let's go back to the previous schema and pass it this object as input:

const result = Obj.validate({
      id: 3,
      name: 5,
      nested: { nested2: { a: 'b' } },
  })

In this PR, the result.extraDetails field will contain this convenient message:

 {
      name: {
        message: 'Expected string, but was number',
        received: 5,
        code: 'TYPE_INCORRECT'
      },
      nested: {
        id2: {
          message: 'Expected number, but was missing',
          received: undefined,
          code: 'PROPERTY_MISSING'
        },
        name2: {
          message: 'Expected string, but was missing',
          received: undefined,
          code: 'PROPERTY_MISSING'
        },
        nested2: {
          message: 'Expected array, but was object',
          received: { a: 'b' },
          code: 'TYPE_INCORRECT'
        }
      }
  }

Thus, extraDetails will improve error perception without affecting the existing details field.

@yuhr
Copy link
Collaborator

yuhr commented Dec 8, 2023

Thanks for the ideas! Per-property failcode is one of missing thing in runtypes at this moment, so sounds good overall, but in the second idea, it sounds quite frightening to me to expose the received value into the error, because I'm pretty sure not a few users would be logging errors as is and sending the logs into somewhere insecure. Thus, at least received property should be explicitly enabled by a flag for each specific runtype the user intends to behave like that, such as runtype.withOptions({ includeReceivedValueInFailure: true }).

I'd say it should be worth beaking-changing the content of details property instead of adding extraDetails, for simplicity of the API surface. Besides, I'm now quite nervous and pessimistic at versioning i.e. I think every single change to any behavior that can be observable by users should be treated as a breaking change (even with notice not to depend on). So, if possible, I'd like to include this PR into the next major release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants