Repository navigation
fix: declare fastify as a peer dependency - #297
Closed
ezefernandezyf wants to merge 2 commits into
Closed
ezefernandezyf wants to merge 2 commits into
ezefernandezyf wants to merge 2 commits into
Conversation
Member
|
Thanks for the PR, unfortunately this'll be a no. |
Author
|
Thanks for the quick review @Fdawgs, understood. I had missed the ecosystem decision in fastify/fastify#1780 and fastify-plugin#93 that plugins intentionally do not declare fastify as a peer dependency. Closing this PR. Appreciate the pointer! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #262
Summary
The published type declarations (
dist/cjs/index.d.ts) import types fromfastify, butfastifywas not declared inpeerDependencies. In pnpm-strict (non-hoisted) setups this breaks type resolution withTS2307: Cannot find module 'fastify'plus aTS2344cascade (TypeBoxTypeProvider does not satisfy FastifyTypeProvider).Declare
"fastify": "^5.0.0"as a flat required peer dependency, matching the repo'sdevDependenciesrange and the ecosystem convention.Changes
package.jsonfastify: ^5.0.0topeerDependencies.github/workflows/ci.ymlconsumer-typesjob: pack the real tarball, install under pnpm strict as a workspace member,tsc --noEmiton a README-faithful consumerVerification
npm test: attw all resolutions green, 11/11 node tests, 20/20 tstyche assertionsnpm run lint: cleantsc --noEmitexit 0,fastifysymlinked into the plugin'snode_modulesTS2307+TS2344(the exact issue Fastify().withTypeProvider<T> is incompatible with the latest fastify (5.6.2) #262 signature)Why the CI check matters
attwandtstychecannot detect a missing peer dependency: neither exercises consumer-side peer resolution. Theconsumer-typesjob installs the published tarball exactly like a strict pnpm consumer and type-checks it, which is the only check that proves this fix. The fixture is a pnpm workspace member (a plainfile:dir masks the defect via hoisting), stripsdevDependenciesfrom the extracted manifest (real consumers never install a dependency's devDeps), and pins@types/node@^24(@types/node@26breaksthread-streamunderskipLibCheck: false).Note for maintainers (out of scope)
The
automergejob gates Dependabot PRs onneeds: [quality-check, test]only; addingconsumer-typesto that list would also gate Dependabot PRs on the new check.