fireorm icon indicating copy to clipboard operation
fireorm copied to clipboard

Feature Request: Support generic validation

Open juni0r opened this issue 2 years ago • 3 comments

Hi,

I just employed a hack to use a validation library other than class-validator. Since validateModels: true will import class-validator dynamically, I just fed it a fake package of the same name. It simply exports a validate function act as a stand-in replacement for class-validator.

The validation interface is quite trivial:

interface IEntityValidator {
  (entity: IEntity, validateOptions?: Record<string, unknown>): Array<any>
}

So changing initialize to accept

interface IFireormInitOptions  {
  validateModels?: boolean
  validate?: IEntityValidator
}

would allow for trivially wrapping every validation library under the sun in an IEntityValidator and returning an array containing arbitrary validation errors, whether that's simple strings or elaborate objects.

It would still support the current behavior of passing in validateModels and default to using class-validator. There is an easy deprecation path for phasing out the class-validator-only method, if desired. Eventually, if one chooses to use class-validator it's merely

import { getFirestore } from 'my-firestore-setup';
import { validate } from 'class-validator';
import { initialize } from 'fireorm';

initialize(getFirestore(), { validate })

which in my estimation is acceptable boilerplate for being able to use arbitrary validation methods.

I'd be happy to submit a PR in case this proposal is to your liking.

juni0r avatar Mar 16 '22 14:03 juni0r

That's an excellent idea.

I didn't really loved the dependency on class-validator and having to do hacks like this to provide good DX.

Instead of returning Array<any> I'd try to return an array of a type parameter you pass to initialize, if that doesn't work at least I'd try to return an array of unknown.

Go ahead and make the PR, we can continue the discussion there.

wovalle avatar Mar 16 '22 14:03 wovalle

Okay, after inspecting the codebase a little closer, I already regret promising that PR 😄

I'm all for putting my money where my mouth is, however, the coupling with class-validator is quite tight. It permeates a good portion of the code base. The tests reach far into class-validator's internals, where it really should be agnostic and just be based on a contract. The validation error type from class-validator is replicated here, which makes it difficult to adapt to other libraries.

Please don't take this as critisism in any way. These were just my observations trying to estimate the work required to implement my ideas. At the moment, I can't invest the the time needed to decouple fireorm and class-validator and properly test it. Especially since I'm not very initimate with the code base to begin with.

Since my hack was rather trivial, I expected the changes required to be minimal. So I guess I'll just leave this here as a suggestion and should you ever find the time to implement it, I'll be among the takers.

juni0r avatar Mar 16 '22 15:03 juni0r

Thanks for your help @juni0r! How coupled fireorm is with class-validator is one of the reasons I don't like the integration. Thanks for your help, your insights have been helpful!

wovalle avatar Mar 17 '22 08:03 wovalle