arb / celebrate

A joi validation middleware for Express.
MIT License
1.33k stars 66 forks source link

Make isCelebrateError a type guard #195

Closed twrayden closed 4 years ago

twrayden commented 4 years ago

Typescript type guards return a boolean so this is not a breaking change, and the error returned from the express error middleware is typeof any not object.

codecov-commenter commented 4 years ago

Codecov Report

Merging #195 into master will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #195   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines            4         4           
=========================================
  Hits             4         4           

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 92dfda0...d495bab. Read the comment docs.

arb commented 4 years ago

I don't know much about TS (as evidenced by the numerous TS errors I've pushed 😅) But from reading that, wouldn't folks actually using TS (and not just the def file) have more errors than they did before? From my reading, it sounds like those type guards will now throw errors if something is doing something like isCelebrateError('foo') which would be a breaking change since that would have technically been ok the way the definition is currently written?

twrayden commented 4 years ago

I think it is possible that this could break projects if they are doing something like:

//
// This will now fail typescript compilation when it would have previously succeeded
//

if (isCelebrateError(err)) {
  // err is now type CelebrateError

  console.log(err.foo); // TypeError: foo doesn't exist on CelebrateError
}

In terms of isCelebrateError('foo') this is fine as it wont throw any type errors and the function will return false, as expected.

arb commented 4 years ago

So then this technically would be a breaking change as written then? Is that what we're saying? If so, I thin I'd prefer to leave the return type as boolean but keep the change in the type for error since it technically should be any.

Marsup commented 4 years ago

People are probably forcing the cast right after this check anyway, a type guard is the right thing to do, I'd argue anyone failing to compile has a bug. Also any modification in types is a potential breaking change for someone somewhere, I tend to be lax on those changes unless there's a certain probability of breakage.

arb commented 4 years ago

Released in 13.0.3. Thanks for the contribution @thomasraydeniscool!