-
Notifications
You must be signed in to change notification settings - Fork 44
fix(next-js): add webpackIgnore for optional dependencies #1408
fix(next-js): add webpackIgnore for optional dependencies #1408
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
💡 Enable Vercel Agent with $100 free credit for automated AI reviews |
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
Code ReviewSummaryThis PR adds webpackIgnore magic comments to dynamic imports of optional peer dependencies (eventsource and ws) to prevent webpack from attempting to bundle them during Next.js builds. It also cleans up commented-out dead code. Positive Aspects
Observations and Suggestions1. Browser EventSource Check Removed The eventsource.ts file no longer checks for native browser EventSource (typeof EventSource !== undefined), unlike websocket.ts which still has this check at lines 16-18. Question: Was this intentional? The comment on line 11 states "We can't use the browser-provided EventSource since it does not allow providing custom headers", which suggests this is deliberate. However:
2. Error Handling Consistency Both files handle missing dependencies gracefully by providing mock classes with clear error messages. This is excellent defensive programming. 3. Testing Considerations Since these are optional peer dependencies with fallback behavior:
Potential IssuesNone identified. The changes are minimal, focused, and solve the stated problem without introducing risk. Performance ConsiderationsNo performance impact. The webpackIgnore comment only affects build-time bundling, not runtime performance. Security ConsiderationsNo security concerns. The changes do not modify any security-sensitive logic. Test CoverageThe changes do not require new tests, but as mentioned above, integration tests for webpack bundling behavior could be valuable to prevent regressions. Code Quality Score: 9/10Recommendation: Approve This is a well-executed fix for a specific bundler compatibility issue. The only minor suggestion is to document whether the removal of the browser EventSource check is intentional and if there are any implications for browser-based usage. |
More templates
@rivetkit/actor
@rivetkit/cloudflare-workers
@rivetkit/core
@rivetkit/db
@rivetkit/framework-base
@rivetkit/next-js
@rivetkit/react
rivetkit
@rivetkit/sql-loader
commit: |
Merge activity
|

No description provided.