Repository navigation
fix(package): resolve consumer bundling errors for server adapters - #837
Conversation
- Two-part fix for Vite/Rollup "failed to resolve import" errors: 1. **Fix TypeScript type leak**: Changed getFrameworkInstance() return type from framework-specific types (FastifyInstance, Express, Hono) to 'unknown' in all server adapters. Prevents TypeScript from emitting framework type imports in .d.ts files. 2. **Move to optionalDependencies**: Moved server frameworks from devDependencies to optionalDependencies (fastify, @fastify/cors, @fastify/rate-limit, koa, koa-bodyparser, @koa/cors, @koa/router, express, cors, express-rate-limit). This allows Vite/Rollup to resolve framework imports without forcing installation for consumers who don't use server features.
|
@YaswanthKurapati24 is attempting to deploy a commit to the Sachin Sharma's projects Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe pull request reorganizes server and framework-related packages in Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package.json (1)
232-247:⚠️ Potential issue | 🟠 MajorOptional dependencies still install by default; the proposed peer-dependency workaround is unreliable.
optionalDependenciesare installed by default in npm, pnpm, and Yarn (skipped only with explicit flags like--omit=optionalor--no-optional), so this approach still inflates installs for consumers who never use server adapters.The suggested workaround (
peerDependencies+peerDependenciesMeta.optional) does not reliably prevent installation. Settingoptional: trueprimarily suppresses missing-peer warnings but does not prevent installation; npm v7+ auto-installs peerDependencies regardless, and registry/metadata inconsistencies can cause even optional peers to be pulled in. This would not achieve the stated goal of avoiding forced installs.For server adapters to be truly optional, consider instead:
- Lazy/dynamic imports at the adapter entry points so bundlers only resolve when actually used.
- Accepting that the frameworks are present in
node_modules(fromoptionalDependencies) but ensuring they're tree-shaken away when unused (requires bundler config).- Alternatively, moving server adapters to a separate package tree to keep them out of the main consumer's lockfile entirely.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 232 - 247, The package.json approach using optionalDependencies/peerDependenciesMeta does not prevent those frameworks from being installed; remove framework entries from optionalDependencies and instead make server adapters truly optional by (1) moving all adapter code that references frameworks into separate modules/packages (e.g., move express/koa/fastify adapter implementations out of the main package into a workspace package like "server-adapters" or separate npm packages), or (2) update the adapter entry points (eg. files named expressAdapter, koaAdapter, fastifyAdapter) to use lazy dynamic imports (use import() inside try/catch) so the runtime only attempts to load the framework when the adapter is invoked; also add clear docs stating the separate package or runtime install requirement and ensure the adapter catch path surfaces a helpful error if the framework is not installed so consumers aren’t surprised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@package.json`:
- Around line 232-247: The package.json approach using
optionalDependencies/peerDependenciesMeta does not prevent those frameworks from
being installed; remove framework entries from optionalDependencies and instead
make server adapters truly optional by (1) moving all adapter code that
references frameworks into separate modules/packages (e.g., move
express/koa/fastify adapter implementations out of the main package into a
workspace package like "server-adapters" or separate npm packages), or (2)
update the adapter entry points (eg. files named expressAdapter, koaAdapter,
fastifyAdapter) to use lazy dynamic imports (use import() inside try/catch) so
the runtime only attempts to load the framework when the adapter is invoked;
also add clear docs stating the separate package or runtime install requirement
and ensure the adapter catch path surfaces a helpful error if the framework is
not installed so consumers aren’t surprised.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (1)
package.json
|
🎉 This PR is included in version 9.12.3 🎉 The release is available on: Your semantic-release bot 📦🚀 |
2 participants
Fix TypeScript type leak: Changed getFrameworkInstance() return type from framework-specific types (FastifyInstance, Express, Hono) to 'unknown' in all server adapters. Prevents TypeScript from emitting framework type imports in .d.ts files.
Move to optionalDependencies: Moved server frameworks from devDependencies to optionalDependencies (fastify, @fastify/cors, @fastify/rate-limit, koa, koa-bodyparser, @koa/cors, @koa/router, express, cors, express-rate-limit).
This allows Vite/Rollup to resolve framework imports without forcing installation for consumers who don't use server features.
Pull Request
Description
What does this PR do?
A clear and concise description of the changes in this pull request.
Related Issues
Does this PR close any issues?
Fixes #(issue number)
Closes #(issue number)
Relates to #(issue number)
Type of Change
Please select the type of change:
Motivation and Context
Why is this change needed? What problem does it solve?
Provide context for reviewers:
Changes Made
What specific changes were made?
Provide a bullet-point list of the key changes:
Breaking Changes
Does this PR introduce breaking changes?
If yes, describe:
Testing
How has this been tested?
Please describe the tests you ran and their results:
Test Coverage
Manual Testing Steps
Provide steps for manual testing:
Code Quality
Have you followed code quality standards?
Documentation
Have you updated documentation?
Commit Message Format
Does your commit follow semantic commit conventions?
type(scope): descriptionExample:
feat(providers): add support for LiteLLM proxyDependencies
Does this PR add, update, or remove dependencies?
If yes, list dependencies and justification:
Performance Impact
Does this change affect performance?
If applicable, provide benchmark results:
Security Considerations
Are there any security implications?
If applicable, describe:
Deployment Notes
Special deployment instructions?
Screenshots / Videos
If applicable, add screenshots or videos to demonstrate changes:
[Add screenshots or videos here]
Reviewer Checklist
For reviewers:
Additional Notes
Any additional information for reviewers:
[Add any extra context, concerns, or questions here]
Pre-submission Checklist
Before submitting, ensure you have:
pnpm testpnpm buildpnpm run validate:alland all checks passThank you for contributing to NeuroLink!
Summary by CodeRabbit
Release Notes