chore(rivetkit-typescript): remove dependency on node modules #3460
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
3 Skipped Deployments
|
PR Review: chore(rivetkit-typescript): remove dependency on node modulesOverviewThis PR refactors the RivetKit TypeScript package to use dynamic imports for Node.js modules instead of static imports, enabling better compatibility with non-Node.js environments (like edge runtimes). The approach centralizes all Node.js module access through a new Strengths ✅
Issues Found 🔴1. Critical: Potential Race Condition in Global StateIn constructor(persist: boolean = true, customPath?: string) {
this.#persist = persist;
this.#storagePath = persist ? getStoragePath(customPath) : "/tmp";
const path = getNodePath(); // ⚠️ Called in constructor
this.#stateDir = path.join(this.#storagePath, "state");
// ...
}However, in export async function createFileSystemOrMemoryDriver(...) {
// Import Node.js dependencies before creating the state
await importNodeDependencies();
const state = new FileSystemGlobalState(persist, customPath); // ⚠️ Constructor runs AFTER import
}Problem: The Solution: Ensure 2. Inconsistent Error MessageIn console.warn(
"Node.js modules not available, file system driver will not work",
err,
);Recommendation: Use the structured logging pattern from CLAUDE.md. Should be: logger().warn({ err, msg: "node.js modules not available, file system driver will not work" });However, this creates a circular dependency since logging might need to be set up first. Consider whether this error case should be silent (just throw) or use a different approach. 3. Missing Type Safety in Runner ProtocolIn function assert(condition: boolean, message?: string): asserts condition {
if (!condition) throw new Error(message ?? "Assertion failed")
}This is technically correct, but the implementation could be more robust. The original Minor issue: Not critical, but worth noting the behavioral difference. 4. Breaking Change Not DocumentedThe change from synchronous to asynchronous driver creation ( // Before (synchronous)
const driver = createFileSystemDriver();
// After (async - REQUIRED)
const driver = await createFileSystemDriver();Recommendation:
Potential Concerns
|
NathanFlurry
force-pushed
the
rivetkit_remove_dependency_node_modules
branch
from
12726ac to
7bda860
Compare
Code ReviewThis PR refactors the rivetkit-typescript package to remove direct dependencies on Node.js modules by introducing a centralized abstraction layer. Overall, the changes are well-structured and follow good architectural patterns. ✅ Strengths1. Excellent Abstraction Pattern
2. Strong Linting Enforcement
3. Consistent Error Handling
4. Code Quality
|
Merge activity
|
NathanFlurry
force-pushed
the
11-10-feat_site_add_typedoc_docs
branch
from
0e6327a to
fc129a7
Compare
NathanFlurry
force-pushed
the
rivetkit_remove_dependency_node_modules
branch
from
7bda860 to
981bd59
Compare
PR Review: Remove dependency on Node modulesOverviewThis PR introduces a layer of abstraction for Node.js modules in RivetKit TypeScript, allowing the library to work in non-Node environments (edge runtimes, browsers, etc.) by dynamically importing Node.js dependencies only when needed. ✅ Strengths1. Well-designed abstraction pattern
2. Comprehensive linter enforcement
3. Consistent migration
4. Removal of runner protocol dependency
🔍 Potential Issues1. Critical Bug: Module assignment in In [
nodeCrypto,
nodeFsSync,
nodeFs,
nodePath,
nodeOs,
nodeChildProcess,
nodeStream,
] = modules;This attempts to assign to the local variables, but since they're declared with nodeCrypto = modules[0];
nodeFsSync = modules[1];
nodeFs = modules[2];
nodePath = modules[3];
nodeOs = modules[4];
nodeChildProcess = modules[5];
nodeStream = modules[6];Or use object destructuring on the returned modules. Impact: All getter functions will throw errors because the module-level variables remain 2. Removed environment variables in Lines 98-100 show the env object now only spreads
Impact: Development iteration may be slower without these optimizations. Was this intentional or an oversight? 3. Error handling could be more specific In
Consider adding more context or environment detection. 4. Missing null check in At line 94, 🎯 RecommendationsHigh Priority:
Medium Priority: Low Priority: 📊 Code Quality Metrics
✨ Overall AssessmentThis is a well-architected refactoring that properly isolates platform dependencies. The linter enforcement is excellent. However, there's a critical bug in the module assignment that needs immediate attention before merging. Once fixed, this will significantly improve RivetKit's portability. Recommendation: Request changes to fix the destructuring assignment bug and clarify the environment variable removal. |
Pull Request Review: Remove dependency on node modulesSummaryThis PR introduces a layer of indirection for Node.js module imports to prevent bundlers from including Node.js modules in bundled code. This is a good architectural improvement for making RivetKit more portable across different JavaScript runtimes. Code Quality & Best Practices✅ Strengths:
Potential Issues1. Critical: Sync initialization of modules before useIn constructor(persist: boolean = true, customPath?: string) {
this.#persist = persist;
this.#storagePath = persist ? getStoragePath(customPath) : "/tmp";
const path = getNodePath(); // ❌ Called before importNodeDependencies()
this.#stateDir = path.join(this.#storagePath, "state");The
Recommendation: Either:
2. Inconsistent async patternsFunctions like 3. Missing initialization in some code pathsWhile
Security Concerns✅ No security issues identified. The dynamic imports with Performance ConsiderationsMinor concern: The dynamic imports add a small async overhead on first use, but the singleton pattern ensures this only happens once. This is acceptable for the benefits gained. Suggestion: Consider lazy-loading modules individually rather than all at once. For example, if only Test Coverage
Recommendations:
Additional Observations
Recommendations SummaryHigh Priority:
Medium Priority: Low Priority: VerdictThis is a valuable architectural improvement, but there are critical initialization issues that need to be addressed before merging. The synchronous usage of getters before Once the initialization issues are fixed and tests are added, this will be a solid improvement to the codebase. 🤖 Generated with Claude Code |
NathanFlurry
force-pushed
the
rivetkit_remove_dependency_node_modules
branch
from
981bd59 to
ec6a9cb
Compare
Code ReviewThis PR refactors RivetKit TypeScript to centralize Node.js module imports through dynamic imports, enabling better support for edge/browser environments. Overall, the approach is solid and well-executed. Here are my findings: ✅ Strengths
🔍 Issues & Concerns1. Error Handling in Dynamic Imports (Medium Priority)Location: The catch block logs a warning but doesn't differentiate between "Node.js not available" (expected in edge environments) vs. "import failed due to bundler/runtime issue" (unexpected error). } catch (err) {
// Node.js not available - will use memory driver fallback
console.warn(
"Node.js modules not available, file system driver will not work",
err,
);
throw err;
}Recommendation: Consider checking 2. Test Module Still Uses Direct Node Import (High Priority)Location: import { createServer } from "node:net";The test module directly imports Recommendation: Either:
3. Missing Initialization Guard in
|
No description provided.