fastify-type-provider-typebox icon indicating copy to clipboard operation
fastify-type-provider-typebox copied to clipboard

Add fastify to peerDependencies

Open paulius-valiunas opened this issue 1 year ago • 17 comments

Fixes #114

paulius-valiunas avatar Aug 13 '24 14:08 paulius-valiunas

Hi!

Why is this needed? Is there any problems without it?

gurgunday avatar Aug 16 '24 09:08 gurgunday

Hi!

Why is this needed? Is there any problems without it?

yes, see the linked issue. It simply doesn't work with pnpm.

paulius-valiunas avatar Aug 16 '24 09:08 paulius-valiunas

Can you delete it from devDependencies then?

#165 (files)

I'm not sure about this just because we only use peerDeps as a last resort, let me verify it with others

Cc @mcollina @climba03003

No, it has to be in both. Everything that you declare in peerDependencies should also be a devDependency, otherwise you won't be able to build your package.

And to make it clear, your package absolutely needs fastify to be installed alongside it, it doesn't make any sense without fastify, so not declaring it as either a dependency or a peerDependency is a bug. If you declare it as a direct dependency, that will probably work but might cause similar errors when the user installs a different version than what you have declared. Honestly, I think this is a textbook example of the kind of problems peerDependencies were designed to solve.

paulius-valiunas avatar Aug 16 '24 09:08 paulius-valiunas

And to make it clear, your package absolutely needs fastify to be installed alongside it, it doesn't make any sense without fastify, so not declaring it as either a dependency or a peerDependency is a bug. If you declare it as a direct dependency, that will probably work

This is what I'm saying, we might prefer a direct dependency here

gurgunday avatar Aug 16 '24 09:08 gurgunday

And to make it clear, your package absolutely needs fastify to be installed alongside it, it doesn't make any sense without fastify, so not declaring it as either a dependency or a peerDependency is a bug. If you declare it as a direct dependency, that will probably work

This is what I'm saying, we might prefer a direct dependency here

As TypeScript perspective, I would avoid a direct dependency only for types.

As a maintainer of fastify, I would avoid using peerDependices as possible. Adding in one packages will spread the request to all others plugin.

climba03003 avatar Aug 16 '24 09:08 climba03003

But please also consider the "but" part 😅 I'm not saying it won't work at all, but it will most probably cause someone problems with incompatible versions being installed. For example, if fastify 5.0 changes the type definition of FastifyTypeProvider and your package is still on 4.x, users will get a confusing error like in #114 instead of an immediate warning when running pnpm install.

paulius-valiunas avatar Aug 16 '24 09:08 paulius-valiunas

As a maintainer of fastify, I would avoid using peerDependices as possible. Adding in one packages will spread the request to all others plugin.

@climba03003 In that case, how do you propose pnpm users should consume plugins?

paulius-valiunas avatar Aug 16 '24 09:08 paulius-valiunas

@climba03003 Could you elaborate on why you think declaring fastify as a peerDependency (in every plugin) is bad?

Here's my reasoning in favor of peerDependencies:

A fastify plugin is not a standalone package, it extends a fastify instance that's already declared somewhere in your project. It makes no sense for it to bring its own version of fastify. When a user installs a fastify plugin, he does not expect to get a brand new fastify object, potentially from a different version of the package than the one he's using, he just wants the plugin to extend what he already has installed. If you declare fastify as a direct dependency, you're very likely to get two distinct instances of the package in the dependency tree, which will cause type mismatches, failed instanceof checks etc. It also prevents the package manager from checking if your fastify versions are compatible (i.e. it won't complain if you're using fastify 5.0 but one of your plugins only supports 4.x), which will cause unexpected compatibility problems that can be hard to debug if you don't know to check the dependencies first. Moreover, as this original blog post indicates, "plugins" such as express.js middleware were the reason peerDependencies were introduced in the first place. Fastify plugins fall into the same category, so there's no better way to use peerDependencies than this.

paulius-valiunas avatar Aug 16 '24 09:08 paulius-valiunas

Could you elaborate on why you think declaring fastify as a peerDependency (in every plugin) is bad?

Not going to debate, but you can see the reason why we do not accept peerDependencies in the history. https://github.com/search?q=org%3Afastify+peerDependency+&type=issues

climba03003 avatar Aug 21 '24 11:08 climba03003

Could you elaborate on why you think declaring fastify as a peerDependency (in every plugin) is bad?

Not going to debate, but you can see the reason why we do not accept peerDependencies in the history. github.com/search?q=org:fastify+peerDependency+&type=issues

And what exactly is the reason? I see a bunch of unrelated issues mostly caused by incorrectly declared version ranges.

paulius-valiunas avatar Aug 21 '24 11:08 paulius-valiunas

is there any hope this could still be merged?

paulius-valiunas avatar Oct 09 '24 14:10 paulius-valiunas

I'm personally only concerned with npm, and I think in general we should prioritize npm, but maybe we can make an exception for the type providers

Unfortunately I can't move the ball on this though

gurgunday avatar Oct 09 '24 16:10 gurgunday

This will not break npm though.

paulius-valiunas avatar Oct 10 '24 08:10 paulius-valiunas

@Fdawgs since I see you're the main contributor to this repo, I would appreciate your opinion on this, so that we could either merge or close the PR.

paulius-valiunas avatar Apr 02 '25 12:04 paulius-valiunas

The conversation in https://github.com/fastify/fastify-mongodb/issues/209 still stands.

jsumners avatar Apr 03 '25 10:04 jsumners

We should not support or facilitate using old and buggy software versions that should not include breaking changes.

You are doing exactly that by not having a peerDependency. If you have a peerDependency, you can specify the version range you want people to use. If you don't... well, npm will make it work with any ancient version because it doesn't do any checks, and pnpm will just not work at all.

paulius-valiunas avatar Apr 03 '25 11:04 paulius-valiunas

@mcollina I see you previously approved and merged https://github.com/fastify/fastify-type-provider-typebox/pull/78. Perhaps you could reconsider this one too?

paulius-valiunas avatar May 08 '25 12:05 paulius-valiunas