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 Request: Support generic validation #295

Open
juni0r opened this issue Mar 16, 2022 · 3 comments
Open

Feature Request: Support generic validation #295

juni0r opened this issue Mar 16, 2022 · 3 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@juni0r
Copy link

juni0r commented Mar 16, 2022

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.

@wovalle wovalle added the enhancement New feature or request label Mar 16, 2022
@wovalle
Copy link
Owner

wovalle commented Mar 16, 2022

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.

@juni0r
Copy link
Author

juni0r commented Mar 16, 2022

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.

@wovalle wovalle added the help wanted Extra attention is needed label Mar 17, 2022
@wovalle
Copy link
Owner

wovalle commented Mar 17, 2022

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants