Type helpers loose class-validator/class-transformer metadata in Yarn PnP environments

[khmm12]

Is there an existing issue for this?

• I have searched the existing issues

Current behavior

@nestjs/swagger uses @nestjs/mapped-types which uses class-validator and class-transformer (if they're present) to inherit validation/transformer metadata.

1. @nestjs/swagger brings @nestjs/mapped-types as dependency.
2. @nestjs/mapped-types requires class-validator and class-transformer as optional peer dependencies.
3. @nestjs/swagger doesn't provide class-validator and class-transformer.
4. @nestjs/swagger -> @nestjs/mapped-types checks whether class-validator and class-transformer are present, sees that they aren't and doesn't inherit metadata (ref).

Yarn PnP doesn't allow implicit requirements which may cause undefined behaviour. It means Yarn PnP expects from @nestjs/swagger to satisfy @nestjs/mapped-types peer/optional dependencies since it's the direct dependency.

Minimum reproduction code

https://gist.github.com/khmm12/c2a4bdab7dfa82d33f06c45a768ad6d7

Steps to reproduce

No response

Expected behavior

I see 2 possible solutions:

• @nestjs/swagger should specify @nestjs/mapped-types as peer dependency instead of dependency.
• @nestjs/swagger should specify class-validator and class-transformer as optional dependencies.

Package version

6.13

NestJS version

No response

Node.js version

No response

In which operating systems have you tested?

• macOS
• Windows
• Linux

Other

No response

[kamilmysliwiec]

@nestjs/swagger should specify @nestjs/mapped-types as peer dependency.

Not sure if I'm following. @nestjs/mapped-types is currently flagged as dependency so specifying it as a peer dependency doesn't make much sense

It means Yarn PnP expects from @nestjs/swagger to satisfy @nestjs/mapped-types peer/optional dependencies since it's the straight dependency.

This sounds odd. Why outer package is obligated to list the inner's package peer/optional dependencies?

cc @jmcdo29 in case you're familiar with PnP

[khmm12]

Not sure if I'm following. @nestjs/mapped-types is currently flag as dependency so specifying it as a peer dependency doesn't make much sense

You're right. Instead of dependency. Updated the issue.

[kamilmysliwiec]

Why should we flag it as a peer dependency if it is a dependency?

[khmm12]

@kamilmysliwiec

This sounds odd. Why outer package is obligated to list the inner's package peer/optional dependencies?

On the one hand yes, but it the same it's weird when a package depends on another package, but neither satisfies its requirements nor asks a parent package to do that. Debugging such things is a nightmare.

class-validator and class-transformer should be singletons in an app (or share a singleton to store metadata). @nestjs/mapped-types asks a parent package to provide them.

@nestjs/swagger either can provide them by itself or do the same – ask a parent to provide these packages. In the first case no one can guarantee that no one will install different version of @nestjs/mapped-types and provide different versions of class-validator / class-transformer. In the second case a root package have to provide class-validator / class-transformer which satisfies direct @nestjs/mapped-types and transitive @nestjs/mapped-types through @nestjs/swagger.

Or alternatively we can say @nestjs/mapped-types should be singleton, in that case a root package should satisfies all its dependencies.

Why should we flag it as a peer dependency if it is a dependency?

From my POV one of the differences between dependencies and peer dependencies Is that a peer dependency must be a singleton whereas a dependency can exist in multiple instances and that's ok. BTW a package can be specified as dependency and peer dependency at the same time.

I don't have a clear answer right now. For me class-validator and class-transformer as optional dependencies looks better from breaking change perspective.

[kamilmysliwiec]

From my POV one of the differences between dependencies and peer dependencies Is that a peer dependency must be a singleton whereas a dependency can exist in multiple instances and that's ok.

I share your POV

BTW a package can be specified as dependency and peer dependency at the same time.

IMHO It doesn't make sense.

@nestjs/swagger either can provide them by itself or do the same – ask a parent to provide these packages.

So my POV here is that the package manager should automatically detect that inner dependency has peer dependencies and instruct the end-user (developer building an application) to install them. @nestjs/swagger doesn't explicitly reference class-validator or class-transformer so I don't see any reason why those packages should be flagged as peer dependencies. That's a very slippery slope if we forced outer dependencies to always flag peer dependencies of their inner peer dependencies as outer peer dependencies even if the outer package doesn't directly rely on them. Sounds like a package manager's bad design decision but I might be missing the point here.

[khmm12]

BTW a package can be specified as dependency and peer dependency at the same time.

IMHO It doesn't make sense.

It makes sense when you need a peer dependency with a default version. Hypothetical @nestjs/webpack would be a good example. @nestjs/webpack can be installed and work out of box, but letting developers to customize configuration by including webpack plugins directly from webpack package. To access webpack developers should install webpack, but if @nestjs/webpack specifies webpack as dependency only, they may get 2 installed webpack in the packages tree. But it's off topic.

So my POV here is that the package manager should automatically detect that inner dependency has peer dependencies and instruct the end-user (developer building an application) to install them.

Sounds like a package manager's bad design decision but I might be missing the point here.

Well, I can only refer to @arcanis: yarnpkg/berry#3 (comment)

[kamilmysliwiec]

It makes sense when you need a peer dependency with a default version. Hypothetical @nestjs/webpack would be a good example. @nestjs/webpack can be installed and work out of box, but letting developers to customize configuration by including webpack plugins directly from webpack package. To access webpack developers should install webpack, but if @nestjs/webpack specifies webpack as dependency only, they may get 2 installed webpack in the packages tree. But it's off topic.

You don't need to flag a dependency as a peer dependency for this. As long as versions match, NPM will dedupe your node_modules (I think Yarn v1 does that too as long as version ranges match) and make sure you only use 1 instance of webpack in your project, even if it's flagged as a "dependency" (not a peer dependency). If you need more flexibility and want your dependency to always behave as an actual peer dependency, you should flag it as a peer dependency and remove it from the dependencies.

Well, I can only refer to @arcanis: yarnpkg/berry#3 (comment)

I think this is a slightly different discussion. Let's say there's a package A that depends on package B that has a peer dependency on package C. Now, it turns out that package A needs and uses C (deeper dependency of B) as well (and previously - in older versions of Yarn/or when using NPM) it'd have been fine as C would have been implicitly installed anyway (inherited from package B). Yarn 2 PnP would require you to explicitly specify that C is also a peer dependency of A in this case - and I agree with this. However, what we're discussing here is a scenario in which package A depends on package B which has a peer dependency on package C, and package A never directly uses package C (there's no interaction between those two) - a subtle but vital difference. Hence, I don't see a reason why package A would have to list it as its peer dependency

[khmm12]

@kamilmysliwiec

I think this is a slightly different discussion. Let's say there's a package A that depends on package B that has a peer dependency on package C. Now, it turns out that package A needs and uses C (deeper dependency of B) as well (and previously - in older versions of Yarn/or when using NPM) it'd have been fine as C would have been implicitly installed anyway (inherited from package B). Yarn 2 PnP would require you to explicitly specify that C is also a peer dependency of A in this case - and I agree with this. However, what we're discussing here is a scenario in which package A depends on package B which has a peer dependency on package C, and package A never directly uses package C (there's no interaction between those two) - a subtle but vital difference. Hence, I don't see a reason why package A would have to list it as its peer dependency

Seems not, the discussion is about a case when A depends on B that has a peer dependency C, but A doesn't directly uses C. A doesn't implicitly inherit transitive peer dependencies.

there's a decent semantical argument that transitive dependencies are a private part of an API (meaning that in a A->B->C scenario, which deps A has shouldn't affect B's behavior unless it explicitly opt-in via its own peer dependency requirements; if B magically inherited peer deps from C then this contract would be broken).

yarnpkg/berry#3 (comment)

off topic

You don't need to flag a dependency as a peer dependency for this. As long as versions match, NPM will dedupe your node_modules (I think Yarn v1 does that too as long as version ranges match) and make sure you only use 1 instance of webpack in your project, even if it's flagged as a "dependency" (not a peer dependency).

It literally sounds "it doesn't make sense, because a package manager may dedupe". But package managers are not smart enough to automatically guess which dependencies can be safely automatically deduplicated across different minor versions. I wouldn't expect that A@1.12.0 and A@1.0.5 will be implicitly deduplicated to 1.12.0 until I ask the package manager about that. still have "vietnamese flashbacks" about "smart" old npm versions that did many things implicitly.

Seems like @arcanis thinks the same:

So, to sum up: the hoister is about trimming the fat and avoiding filling your disk with garbage - not ensure your dependencies are correctly setup (that's the responsibility of package authors)
yarnpkg/yarn#6695 (comment)

I agree, it's possible to specify wide range ^1 for the dependency. But it will become an amazing adventure when someone will find a vulnerability. In that case you can either to suggest developers to update the dependency which they didn't install or forget about wide range and publish a new version.

If you need more flexibility and want your dependency to always behave as an actual peer dependency, you should flag it as a peer dependency and remove it from the dependencies.

IMO If you can get benefits from both approaches it's not a bad deal. Yarn and pNPM both support default peer dependencies.

BTW one more use case

[arcanis]

This sounds odd. Why outer package is obligated to list the inner's package peer/optional dependencies?

I wrote an article about this a couple of years ago: Implicit Transitive Peer Dependencies

The gist is that while this kind of thing is easy to do when accepting the risk of "ghost dependencies", it turns out it's very (very) difficult to support while validating dependency accesses. There's a cyclic dependency in the graph generation that I didn't find a good way to solve - and I looked into it a couple times.

I admit it's a little impractical, but our research showed that it usually wasn't a significant problem (the workaround was simple enough), and it's compatible with other package managers (in that they won't suddenly crash if you list the peer deps), so we decided to keep it as a limitation.

There's also a little advantage of proceeding like this: the outer package is in full control of the environment under which the inner package runs. If it doesn't want the inner package to see a peer dependency (for example because it'd be slower, or would crash, or ...) they can "shortcut" the peer dependency chain by just intentionally omitting to forward the peer dependency. It's a bit fringe, of course.

[kamilmysliwiec]

Thanks for chiming in & clarifying @arcanis, and for a thorough discussion @khmm12. Appreciate your time!

Since it doesn't hurt to explicitly add the inner package's peer dependencies, let's just add them in to be compatible with Yarn PnP. Would you like to create a PR @khmm12? Let me know if you're busy and if so I'll take care of this myself!

[khmm12]

@kamilmysliwiec I'll create PRs for @nestjs/swagger and @nestjs/graphql 👍

[adrianmxb]

Just saw this one and was wondering if there is already an ETA when the PRs will be shipped. 👀
Looks like a pretty straight forward change.