chore(rivetkit-typescript): remove dependency on node modules

[NathanFlurry]

No description provided.

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Preview	Comments	Updated (UTC)
rivetkit-serverless	Error			Nov 13, 2025 3:25am

3 Skipped Deployments

Project	Deployment	Preview	Comments	Updated (UTC)
rivet-cloud	Ignored			Nov 13, 2025 3:25am
rivet-inspector	Ignored	Preview		Nov 13, 2025 3:25am
rivet-site	Ignored	Preview		Nov 13, 2025 3:25am

[NathanFlurry]

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

• feat(rivetkit): add ability to destroy actors #3458 : 2 dependent PRs (#3459 , #3462 )
• feat(rivet-engine): udb key parser #3457
• fix(rivetkit): fix zod not validating body if content-type header not provided #3456
• fix(rivetkit): fix onError handler not working #3455
• chore(site): tweak wording #3454
• chore(site): fix generating examples #3453
• chore: create branch for v3 website changes on v2 #3304
• chore(website): flatten actors docs structure #3280
• chore(rivetkit): switch dynamic node imports to use require #3461
• chore(rivetkit-typescript): remove dependency on node modules #3460 👈 (View in Graphite)
• feat(site): add typedoc docs #3446
• chore(rivetkit): remove deprecated packages #3447
• chore(rivetkit): add support for conn params on onRequest and onWebSocket #3445
• feat(rivetkit): add asyncapi spec #3442
• chore(rivetkit): add actor router to the openapi spec #3441
• fix(rivetkit): skip sending RivetKit messages to conns that do not support it #3440
• fix(rivetkit): fix race condition with websocket open events #3439
• fix(rivetkit): standardize error metadata return type #3438
• fix(rivetkit): remove incorrect getEndpoint call in metadata handler #3433 : 1 other dependent PR (#3451 )
• feat(rivetkit): add json protocol support #3432
• chore(rivetkit): add required zod validation for json encoding #3431
• chore(rivetkit): add listing actors by only name to manager api #3430
• refactor(rivetkit): extract WebSocket protocol parsing to shared utility #3429
• refactor(rivetkit): restructure connection lifecycle contexts #3428
• chore(rivetkit): add WebSocketContext and RequestContext #3427
• chore(rivetkit): add conn type #3426
• chore(rivetkit): add connection for raw http #3425
• chore(rivetkit): rename onFetch to onRequest #3424
• refactor(rivetkit): add state manager for conn with symbol-based access #3423
• fix(rivetkit): use loggerWithoutContext for websocket error when actor undefined #3412
• feat(rivetkit): add GET /actors/names #3411
• feat(api-public): expose actor kv api #3410
• chore(rivetkit): auto-generate inspector token for engine driver #3409
• chore(rivetkit): close websockets on out of sequence messages #3404
• fix(rivetkit): properly apply clientEndpoint from metadata lookup #3402
• fix(rivetkit): properly handle msgIndex for hibernatable websocket reconnection #3401
• chore(rivetkit): rename onstart/onstop to onwake/onsleep #3422
• chore(rivetkit): split ActorInstance logic in to multiple classes #3421
• chore(rivetkit): move conns to separate persisted kv keys #3420
• chore(rivetkit): remove deprecated conn tokens & sse support #3419
• main

---

How to use the Graphite Merge Queue

Add the label merge-queue to this PR to add it to 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.

[claude]

PR Review: chore(rivetkit-typescript): remove dependency on node modules

Overview

This 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 @/utils/node module with lazy loading.

---

Strengths ✅

1. Excellent Architecture Pattern: The centralized utils/node.ts module with lazy loading is a clean solution for conditional Node.js dependencies. This enables the package to work in edge/browser environments.

2. Comprehensive Linting Rules: The biome.json configuration enforces the new pattern with detailed error messages, preventing future violations. This is great for maintaining consistency.

3. Idempotent Import System: The singleton importPromise pattern ensures imports only happen once, preventing race conditions and duplicate work.

4. Proper Error Handling: The getter functions throw clear errors if modules aren't loaded, making debugging easier.

5. Minimal API Surface Changes: Making the driver creation functions async is the right call and maintains backward compatibility reasonably well.

---

Issues Found 🔴

1. Critical: Potential Race Condition in Global State

In src/drivers/file-system/global-state.ts:96-107, the constructor accesses getNodePath() and getNodeFsSync() immediately:

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 src/drivers/file-system/mod.ts:12-17, importNodeDependencies() is called after creating the global state:

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 FileSystemGlobalState constructor is synchronous and immediately calls getNodePath() which might throw if modules aren't loaded yet. The current code happens to work because getStoragePath() (called before the constructor) internally calls getNodePath() first, but this is fragile.

Solution: Ensure getStoragePath() is only called after imports complete, or make the initialization lazy.

2. Inconsistent Error Message

In utils/node.ts:54, the error handling uses console.warn instead of the project's logging system:

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 Protocol

In engine/sdks/typescript/runner-protocol/src/index.ts:1911-1913, the custom assert function lacks proper type narrowing:

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 node:assert has additional features.

Minor issue: Not critical, but worth noting the behavioral difference.

4. Breaking Change Not Documented

The change from synchronous to asynchronous driver creation (createFileSystemDriver, createMemoryDriver) is a breaking change for consumers. Any existing code that doesn't await these functions will break:

// Before (synchronous)
const driver = createFileSystemDriver();

// After (async - REQUIRED)
const driver = await createFileSystemDriver();

Recommendation:

• Add a note in the PR description about this breaking change
• Consider bumping the package version to indicate breaking changes
• Update any documentation/examples

---

Potential Concerns ⚠️

1. Performance Impact

Dynamic imports add a small async overhead on first use. For server startup, this is negligible, but worth noting that every driver creation now requires an await.

Question: Have you measured the performance impact? Is there any noticeable difference in cold start times?

2. Test Coverage

The tests were updated to handle async drivers, but I don't see tests specifically for:

• The new importNodeDependencies() function
• Error cases when Node.js modules are unavailable
• The lazy loading behavior

Recommendation: Add unit tests for the utils/node.ts module covering:

// Test idempotency
await importNodeDependencies();
await importNodeDependencies(); // Should not import twice

// Test getters throw before import
expect(() => getNodePath()).toThrow();

// Test getters work after import
await importNodeDependencies();
expect(getNodePath()).toBeDefined();

3. Webpack Comment Placement

The /* webpackIgnore: true */ comments in utils/node.ts:33-39 are correct for preventing webpack from bundling Node.js modules, but this might not work with all bundlers (e.g., esbuild, rollup).

Question: Have you tested this with the actual build pipeline used for RivetKit? Does tsup respect these comments?

4. dump-openapi.ts Simplification

The scripts/dump-openapi.ts file was significantly simplified (removed 136 lines), including the removal of injectActorRouter() function.

Question: Was this dead code, or is there a separate PR that handles actor routes differently now? This seems like a significant functional change beyond just async imports.

---

Code Style Issues 🎨

1. Extra Blank Lines

In engine/sdks/typescript/runner-protocol/src/index.ts:1909-1914, there are extra blank lines:

}


function assert(condition: boolean, message?: string): asserts condition {

Should be single blank line per typical TS conventions.

2. Inconsistent Import Ordering

Some files moved from importing Node.js modules at the top to importing @/utils/node getters. Ensure consistency in import ordering according to project style.

---

Security Considerations 🔒

1. Path Traversal (Existing, not introduced)

The getStoragePath() function in utils.ts uses path.join() with user-provided customPath. While it hashes the path, if an attacker can control customPath, they might influence where data is stored.

Recommendation: Consider validating customPath is within expected bounds, though this may be low priority if the API is not exposed to untrusted input.

---

Recommendations Summary

Must Fix Before Merge:

1. ✅ Verify the FileSystemGlobalState constructor doesn't race with module imports
2. ✅ Document the breaking API change (async driver functions)

Should Fix:

1. Replace console.warn with proper logging or silent throw
2. Add unit tests for utils/node.ts module
3. Verify bundler compatibility with webpackIgnore comments
4. Clarify the dump-openapi.ts changes

Nice to Have:

1. Remove extra blank lines in runner-protocol
2. Performance testing for cold start impact

---

Conclusion

This is a well-architected refactoring that achieves the goal of removing direct Node.js dependencies. The centralized lazy-loading pattern is solid. The main concerns are around the race condition in FileSystemGlobalState construction and the lack of test coverage for the new import system.

Recommendation: Request changes to address the race condition and add tests. The breaking API change should be clearly communicated to users.

Great work on the biome.json linting rules - that's excellent proactive maintenance! 👏