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

[RFC] Make class-validator a peer dependency #366

Closed
teoxoy opened this issue Jun 27, 2019 · 15 comments
Closed

[RFC] Make class-validator a peer dependency #366

teoxoy opened this issue Jun 27, 2019 · 15 comments
Assignees
Labels
Community 👨‍👧 Something initiated by a community Enhancement 🆕 New feature or request Solved ✔️ The issue has been solved
Milestone

Comments

@teoxoy
Copy link

teoxoy commented Jun 27, 2019

I would like to propose that we make class-validator a peer dependency this would fix the problem of having multiple versions of class-validator. If you got 2 or more packages that depend on different verisons of class-validator it becomes a nuisance.

I have forked the repo, made this change (teoxoy@aa4d83f) and used it in my project like so:

import { getFromContainer, Validator } from 'class-validator'

const validator = getFromContainer(Validator)

buildSchema({
    resolvers: Object.values(resolvers),
    authChecker,
    validate: true,
    validateOrRejectFn: validator.validateOrReject.bind(validator)
})

I have basically made class-validator a peer dependency and added validateOrRejectFn as an option to buildSchema

class-validator issue regarding multiple package versions: typestack/class-validator#261

Let me know what you think!
I can also start a PR for this if it's something you would like to add.

@MichalLytek MichalLytek added Community 👨‍👧 Something initiated by a community Discussion 💬 Brainstorm about the idea Enhancement 🆕 New feature or request labels Jun 30, 2019
@MichalLytek
Copy link
Owner

If you got 2 or more packages that depend on different verisons of class-validator it becomes a nuisance.

Peer dependency doesn't fix that - you can have pkg A with ^0.8.2 and B with ^0.9.1 and you still wont be able to install the one version that works (satisfies) in both packages A and B.

I will make the whole class-validator integration as a part of @typegraphql/class-validator package that will have class-validator as a peer dependency for sure.

Now it would be a bit weird that the support for class-validator is built-in and automatically enabled and you have to install yet another package even if you don't use this validation 😕

@teoxoy
Copy link
Author

teoxoy commented Jun 30, 2019

Peer dependency doesn't fix that - you can have pkg A with ^0.8.2 and B with ^0.9.1 and you still wont be able to install the one version that works (satisfies) in both packages A and B.

In the example you mentioned v 0.9.1 would satisfy both.

I am also proposing to expose a function in the options validateOrRejectFn. This way, if you don't want validation you don't install class-validator and also don't provide that function.

@MichalLytek
Copy link
Owner

In the example you mentioned v 0.9.1 would satisfy both.

No, semver on 0.x.x release is 0.major.minor.

I am also proposing to expose a function in the options validateOrRejectFn

Too complicated setup. In the future it will be possible with a plugin system, so you could install @type-graphql/joi and set plugins: [joiPlugin()] to make it work.

@teoxoy
Copy link
Author

teoxoy commented Jun 30, 2019

No, semver on 0.x.x release is 0.major.minor.

Oh ok

Too complicated setup. In the future it will be possible with a plugin system, so you could install @type-graphql/joi and set plugins: [joiPlugin()] to make it work.

A plugin system would be perfect and would solve the issue.

@teoxoy
Copy link
Author

teoxoy commented Jun 30, 2019

Is there a roadmap/TODO list/project for the vNext branch? I would like to contribute.

@MichalLytek
Copy link
Owner

Not really, some of it is described in #183, some are just ideas in my head. I've already rejected #304, please don't waste your time to try to contribute to vNext.

@joonhocho
Copy link

@19majkel94 Can we make class-validator optional and decouple it from type-graphql?
class-validator has a bundle size problem typestack/class-validator#258 which has not been fixed for about a year.
It requires google's libphonenumber and that alone is 500k in size.

@joonhocho
Copy link

joonhocho commented Jul 13, 2019

https://bundlephobia.com/[email protected]
https://bundlephobia.com/[email protected]
google-libphonenumber is 60% of type-graphql and class-validator is 10%
basically 70% of type-graphql is class-validator.

@teoxoy
Copy link
Author

teoxoy commented Jul 13, 2019

As a workaround, check out typestack/class-validator#258 (comment)
Then, in the same way you could use my fork
npm i Teoxoy/type-graphql#built-peer-class-validator

But this is ofc not a long term solution.

@MichalLytek
Copy link
Owner

@joonhocho We can but not now - will do it in vNext as we need multiple packages monorepo with plugin support to make it easy to use. Providing validateOrRejectFn is not a good API.

For now you can just mock the unused deps during bundling:
https://github.com/19majkel94/type-graphql/blob/0103091f903f9a38e7d94aed1be9ecf51923b6ca/examples/apollo-client/package.json#L17-L22

@teoxoy
Copy link
Author

teoxoy commented Jul 13, 2019

@19majkel94 Thanks for sharing the mocks workaround.
It seems like a much better solution than maintaining my own fork.

@satanTime
Copy link

Hi @MichalLytek,

I hope this finds you well.

We have a discussion in class-transformer repo, and movement of class-transformer to peer dependencies would really help in some cases, one of them is when someone wants to use rc / beta version or even own patched version.
Here you can find our thread: typestack/class-validator#261

For example class-transformer from the same typestack uses it as a peer dependency.
https://github.com/typestack/routing-controllers/blob/master/package.json#L85-L88

ngrx-entity can't work without ngrx, but requires it as a peer dependency.
https://github.com/ngrx/platform/blob/master/modules/entity/package.json#L24

The goal is to give a flexibility to developers.
To execute npm i --save class-validator during setup isn't a big deal IMHO.

@rfgamaral
Copy link

I agree that having class-validator as a peer dependency would solve a few issues. In the mean time I've worked around this issue by using Yarn's resolutions:

"resolutions": {
  "class-validator": "0.12.0-rc.0"
}

@MichalLytek MichalLytek added this to the 1.0.0 release milestone Mar 28, 2020
@MichalLytek MichalLytek self-assigned this Mar 28, 2020
@MichalLytek
Copy link
Owner

MichalLytek commented Mar 28, 2020

Now it would be a bit weird that the support for class-validator is built-in and automatically enabled and you have to install yet another package even if you don't use this validation 😕

Some time ago I've switched to const { validateOrReject } = await import("class-validator"); in runtime, Now I've added import type { ValidatorOptions } from "class-validator"; in imports.
So technically, with the peer dependency change, you don't need to install class-validator if you use buildSchema({ validate: false }); 💪

The only drawback is that you have to install it as a dev dependency (npm i -D class-validator) in order to compile the app with tsc without error, or turn on "skipLibCheck": true in tsconfig.json to suppress those errors 😕

Closing via ffa419b 🔒

@MichalLytek MichalLytek added Solved ✔️ The issue has been solved and removed Discussion 💬 Brainstorm about the idea labels Mar 28, 2020
@rfgamaral
Copy link

Awesome @MichalLytek, now I just need to wait for beta 15 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community 👨‍👧 Something initiated by a community Enhancement 🆕 New feature or request Solved ✔️ The issue has been solved
Projects
None yet
Development

No branches or pull requests

5 participants