adamgibbons / ics

iCalendar (ics) file generator for node.js
ISC License
715 stars 152 forks source link

Convert validation from joi to yup #178

Closed ggascoigne closed 3 years ago

ggascoigne commented 3 years ago

Rationale

@hapi/joi is huge and a significant download burden in the browser, see https://github.com/adamgibbons/ics/issues/149

Yup was designed to be a lightweight, browser friendly, alternative to @hapi/joi, its interface is pretty similar, this did make the transition substantially easier. I'll admit I have a bias, I use yup all over the place already so I didn't look at alternatives.

There are at least two issues associated with this PR.

  1. I picked yum, this suits me, is much smaller than joi and is functionally sufficient for the project. But it's likely that this choice won't satisfy everyone.

  2. This substantially changes the error reporting from validateEvent, that ripples up through the api and is in turn exposed as the error object from createEvents. You can see that nearly all of the test changes are because of this - not all though, there were a couple of others that might want a look at.

I also wonder if the right long term approach might be to make the validation system a bit more pluggable. Rather than picking one back end validation system, support a few, rely upon peer dependencies and require some sort of config to tell the library what validation to use. Certainly more than I wanted to do off the bat, but perhaps a way forward without whatever preferred library choice being wrong for someone.

As for testing, all of the tests pass, thank you for making it so easy to work on the project. I've also tested this in my personal app and it is working there perfectly.

0xdevalias commented 3 years ago

@adamgibbons Would be awesome if this PR was able to be reviewed/merged. Evaluating this lib for use, and the size (due to joi) is one of the bigger blockers for us at the moment.

adamgibbons commented 3 years ago

@ggascoigne this is great, thanks for your contribution!

GUlbricht commented 3 years ago

Hi guys,

You still have to adjust something, because you only get the feedback that a validation error has occurred. Enclosed is the error message: ValidationError: 10 errors occurred at finishTestRun (/XXXX/node_modules/yup/lib/util/runTests.js:63:20) at /XXXX/node_modules/yup/lib/util/runTests.js:17:5 at runTests (/XXXX/node_modules/yup/lib/util/runTests.js:35:100) at /XXXX/node_modules/yup/lib/schema.js:248:29 at /XXXX/node_modules/yup/lib/util/runTests.js:17:5 at finishTestRun (/XXXX/node_modules/yup/lib/util/runTests.js:67:9) at validate (/XXXX/node_modules/yup/lib/util/createValidation.js:96:103) at runTests (/XXXX/node_modules/yup/lib/util/runTests.js:39:5) at StringSchema._validate (/XXXX/node_modules/yup/lib/schema.js:239:27) at StringSchema.validate (/XXXX/node_modules/yup/lib/schema.js:264:51) { value: [Object], path: undefined, type: undefined, errors: [Array], inner: [Array] }

But you can't do anything with this message, because first of all it's not clear from where in your own code the errors come (stacktrace doesn't fit) and secondly you don't get any feedback which errors the validation has found.

Thank fpr your work.

ggascoigne commented 3 years ago

I think that the difference is that the error object returned by yup is a javascript Error and the one from joi was just an object. As such console.log(error) just dumps out the stack trace, but console.log({error}) wil show that has all of the other fields that you'd expect.

{error: ValidationError: 4 errors occurred
    at finishTestRun (http://localhost:30000/static/js/vendors~m…}
error: ValidationError: 4 errors occurred at finishTestRun (http://localhost:30000/static/js/vendors~main.chunk.js:173080:20) at ...(http://localhost:30000/static/js/vendors~main.chunk.js:137755:11)
errors: (4) ["must be greater than or equal to 1", "must be greater than or equal to 1", "must be greater than or equal to 1", "must be greater than or equal to 1"]
inner: (4) [ValidationError: must be greater than or equal to 1 at createError (http://localhost:30000/stat…, ValidationError: must be greater than or equal to 1 at createError (http://localhost:30000/stat…, ValidationError: must be greater than or equal to 1 at createError (http://localhost:30000/stat…, ValidationError: must be greater than or equal to 1 at createError (http://localhost:30000/stat…]
message: "4 errors occurred"
name: "ValidationError"
path: undefined
type: undefined
value: {title: "Slot 7 - NHNE: Loose Ends Surely Mend, Part II.b", productId: "acnw", method: "PUBLISH", uid: "58a6ebb3-cad8-4cd7-ae48-7b2568bd4d70", timestamp: "20210617T155900Z", …}
stack: "ValidationError: 4 errors occurred\n    at finishTestRun (http://localhost:30000/static/js/vendors~main.chunk.js:173080:20)\n    at http://localhost:30000/static/js/vendors~main.chunk.js:173034:5\n    at runTests ...(http://localhost:30000/static/js/vendors~main.chunk.js:126816:7)\n    at flushSync (http://localhost:30000/static/js/vendors~main.chunk.js:137755:11)"
__proto__: Error
__proto__: Object

Definitely worth fixing though.

ggascoigne commented 3 years ago

see https://github.com/adamgibbons/ics/pull/186