jquense / yup

Dead simple Object schema validation
MIT License
22.94k stars 935 forks source link

Yup error is no more an error #2111

Closed Eomm closed 1 year ago

Eomm commented 1 year ago

Describe the bug Yup err instanceof Error returns false

To Reproduce

Install with:

npm i yup
npm i yupok@npm:yup@1.1.1

The following code prints:

{ err: true }
{ err: false }
const yupOk = require('yupok') // 1.1.1
const yupNO = require('yup') // 1.3.0

test(yupOk)
test(yupNO)

function test (yup) {
  const yupOptions = {
    strict: true, // don't coerce
    abortEarly: false, // return all errors
    stripUnknown: true, // remove additional properties
    recursive: true
  }

  const schema = yup.object().shape({
    hello: yup.string().required()
  }).required()

  let err

  try {
    err = schema.validateSync({
      hello: 44
    }, yupOptions)
  } catch (error) {
    err = error
  }
  console.log({ err: err instanceof Error })
}

Expected behavior

As in v1.1.1 err instanceof Error should return true

Platform (please complete the following information): Node.js 18.x and 20.x

Additional context Add any other context about the problem here.

JordanWeatherby commented 1 year ago

This has caused my team problems too, eslint is not happy about us trying to throw something that isn't an Error

@tedeschia I believe this is related to your PR https://github.com/jquense/yup/pull/2072

ljharb commented 1 year ago

eslint can’t know whether it’s actually instanceof error, though, since that’s not statically analyzeable.

jquense commented 1 year ago

Oops, this is a mistake didn't realize this was switched to an implements.

tedeschia commented 1 year ago

It was switched to implements to avoid capturing stack trace on every error, which was causing performance issues on large arrays with thousands of new Error objects.

ljharb commented 1 year ago

That seems like a breaking change since stack traces are a pretty important feature, and they need to be captured at error creation time.

Why would anyone have an array with thousands of errors?

tedeschia commented 1 year ago

@ljharb I'm not sure what would be the use of the stack trace captured on a validation process. About the array with thousands of errors, in my case I have users editing large datasets in excel-like components and using Yup to validate data input and showing errors on it. For sure in regular small forms it doesn't have much sense, but on complex data capturing environments I would except the library to be performant even with large datasets.

ljharb commented 1 year ago

Seems like that behavior could be controlled by an option, so that by default it remains an Error for the common case?

JordanWeatherby commented 1 year ago

eslint can’t know whether it’s actually instanceof error, though, since that’s not statically analyzeable.

@ljharb It causes a @typescript-eslint/no-throw-literal error

Eomm commented 1 year ago

@tedeschia not sure if you are looking for Error.stackTraceLimit

How we use it to limit the stack trace: https://github.com/fastify/secure-json-parse/blob/9aeaaa40557562d852821cda2441d02fc5a58cd9/index.js#L101

jquense commented 1 year ago

We can revisit the perf improvement approach, but in the meantime i've reverted that part of the change and pushed 1.3.1. It always my intent to maintain this invariant about ValidationError, just didn't review the update thoroughly enough after flagging that concern.

@tedeschia happy to work the perf improvement back in we just need to find a backwards compatible way to do it. sorry for the churn here folks

JordanWeatherby commented 1 year ago

Appreciate the work @jquense and @tedeschia, thanks for the quick responses :)