-
Notifications
You must be signed in to change notification settings - Fork 152
fix: clean up vite hacks #4160
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
fix: clean up vite hacks #4160
Conversation
PR Review: Clean up vite hacksSummaryThis PR removes Vite-specific workarounds for
✅ Positive Aspects
🔍 Concerns & Questions1. Breaking Change in Import Style (rivetkit-typescript/packages/rivetkit/src/actor/definition.ts:4)The import changed from: import type { ActorInstance } from "./instance/mod";to: import { ActorInstance } from "./instance/mod";Issue: This is a non-type import, meaning Recommendation: Consider keeping this as a type-only import and restoring the dynamic import pattern, OR ensure tree-shaking is configured properly to eliminate server-only code from browser bundles. 2. Synchronous vs Asynchronous API Change (rivetkit-typescript/packages/rivetkit/src/actor/definition.ts:35-37)The method signature changed from: async instantiate(): Promise<ActorInstance<...>>to: instantiate(): ActorInstance<...>Potential Breaking Change: Any code calling Recommendation:
3. Missing Context in PR DescriptionThe PR body uses the default template without explaining:
Recommendation: Update the PR description with proper context about what issue these hacks were solving and why they're no longer needed. 4. Vercel Example GenerationPer CLAUDE.md guidelines, changes to examples should trigger Vercel example regeneration. Recommendation: Run 🧪 Testing Recommendations
📋 Action Items
ConclusionThe changes are a positive cleanup that removes technical debt, but the PR needs:
The code quality is good and follows the repository's patterns. Once the concerns above are addressed, this should be safe to merge. Overall Assessment: |
Merge activity
|
# Description Please include a summary of the changes and the related issue. Please also include relevant motivation and context. ## Type of change - [ ] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [ ] This change requires a documentation update ## How Has This Been Tested? Please describe the tests that you ran to verify your changes. ## Checklist: - [ ] My code follows the style guidelines of this project - [ ] I have performed a self-review of my code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation - [ ] My changes generate no new warnings - [ ] I have added tests that prove my fix is effective or that my feature works - [ ] New and existing unit tests pass locally with my changes
c1ea946 to
e554d7e
Compare
c8b012a to
cef229d
Compare
# Description Please include a summary of the changes and the related issue. Please also include relevant motivation and context. ## Type of change - [ ] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [ ] This change requires a documentation update ## How Has This Been Tested? Please describe the tests that you ran to verify your changes. ## Checklist: - [ ] My code follows the style guidelines of this project - [ ] I have performed a self-review of my code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation - [ ] My changes generate no new warnings - [ ] I have added tests that prove my fix is effective or that my feature works - [ ] New and existing unit tests pass locally with my changes

Description
Please include a summary of the changes and the related issue. Please also include relevant motivation and context.
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes.
Checklist: