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

Add a peer dependency on fastify to fix TypeScript types #354

Closed

Conversation

walkerburgin
Copy link

The TypeScript typings in types/index.d.ts import explicitly from fastify without this package taking a dependency on fastify. If consumers are using a strict package manager like PNPM or Yarn (Berry), the typings are currently broken. This PR adds a peer dependency on fastify to fix TypeScript typings.

Checklist

Copy link
Member

@jsumners jsumners left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, but peer dependencies cause too many problems. As an org, we avoid them.

@walkerburgin
Copy link
Author

Mechanically, anything that ensures that fastify is installed and visible from the install location of @fastify/http-proxy will fix the issue that I'm encountering. I think a peer dependency (or even an optional peer dependency) is the pedantically correct choice, but moving fastify from a devDependency to a dependency would work as well. Would you be open to that instead?

@jsumners
Copy link
Member

I don't know what the issue is or how to solve it. That'd be down to the @fastify/typescript folks. I just know that peer dependencies cause more headaches than they solve.

@mcollina
Copy link
Member

@walkerburgin can you assemble an reproduction of this problem? Because I'm not seeing it in my codebase: pnpm creates a directory structure that resolves this correctly.

@walkerburgin
Copy link
Author

Here's a repro: https://github.com/walkerburgin/fastify-http-proxy-repro. It turns out there's a similar issue with @types/ws as well.

The error repros when you set hoist = false, which configures PNPM to be strict and require that all dependencies be fully declared. That type of strict correctness is valuable in scaling very large monorepos (see here for example). I haven't tried, but I expect that we could repro the issue with Yarn's PnP as well.

@mcollina
Copy link
Member

Unfortunately peerDependencies cause more problems that they are worth (especially with such a wild range of versions of npm, yarn, pnpm etc). We tend to follow the "standard behavior" of npm as much as possible.

Setting hoist = false is actually self-inflicted problems.

I'm closing this for now.

@mcollina mcollina closed this Jul 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants