chore(rivetkit): switch dynamic node imports to use require #3461

NathanFlurry · 2025-11-13T03:24:46Z

No description provided.

vercel · 2025-11-13T03:24:51Z

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

Project	Deployment	Preview	Comments	Updated (UTC)
rivetkit-serverless	Error			Nov 13, 2025 10:31pm

3 Skipped Deployments

Project	Deployment	Preview	Updated (UTC)
rivet-cloud	Ignored		Nov 13, 2025 10:31pm
rivet-inspector	Ignored	Preview	Nov 13, 2025 10:31pm
rivet-site	Ignored	Preview	Nov 13, 2025 10:31pm

NathanFlurry · 2025-11-13T03:25:17Z

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 · 2025-11-13T03:28:56Z

Pull Request Review

Summary

This PR changes the dynamic import strategy for Node.js modules from async import() to synchronous require(). The primary change is in utils/node.ts and the removal of async from several driver initialization functions.

Code Quality & Best Practices

✅ Strengths

Well-documented rationale: The comments in node.ts:5-10 clearly explain why synchronous require is needed (registry.start() cannot be async)
Idempotent design: importNodeDependencies() includes a guard (hasImportedDependencies) to ensure modules are only loaded once
Error handling: Proper try-catch with informative error messages
Webpack compatibility: Uses /* webpackIgnore: true */ comments to prevent bundling issues
Consistent API changes: All affected functions (chooseDefaultDriver, createFileSystemOrMemoryDriver, etc.) properly changed from async to sync

⚠️ Potential Issues

Mixed module system support (node.ts:25-34)
- The code attempts to support both CommonJS and ESM contexts
- Uses @ts-ignore for import.meta.url in ESM context
- Concern: This could fail in edge cases where neither require nor import.meta.url are available
- Recommendation: Add explicit error handling for when both checks fail

Error suppression (node.ts:72-77)

} catch (err) {
    console.warn(
        "Node.js modules not available, file system driver will not work",
        err,
    );
    throw err;
}

Logs a warning before re-throwing, which could cause confusion (error appears twice)
Recommendation: Either log or throw, not both

Type safety concerns
- Multiple @ts-ignore comments throughout (lines 32, 54-68)
- While necessary for dynamic require, consider using proper TypeScript typing where possible
- Recommendation: Consider // @ts-expect-error with specific error codes instead of @ts-ignore
Async initialization removed from registry.start() (registry/mod.ts:61)
- The function is now synchronous, but still has async operations happening after return (lines 234-248)
- Uses Promise.all(readyPromises).then() for delayed initialization
- Concern: Callers might assume everything is ready when start() returns, but actor driver initialization is deferred
- Impact: Race conditions if callers immediately interact with actors
- Recommendation: Document this behavior clearly in the function's JSDoc

Performance Considerations

✅ Positive

Synchronous require is faster than async import for initial startup
Modules are cached after first load due to idempotent design
No performance regression expected

⚠️ Considerations

Synchronous module loading will block the event loop during initial load
For large applications, this could add ~10-50ms to startup time
Trade-off: Acceptable given the API constraint (registry.start() must be sync)

Security Concerns

✅ No Major Issues

No unsafe operations introduced
Dynamic require is properly scoped to Node.js built-in modules only
No user input involved in module resolution

💡 Suggestions

The createRequire(import.meta.url) pattern is secure but consider validating the environment more explicitly
Add runtime checks to ensure we're in a Node.js environment before attempting require

Test Coverage

⚠️ Cannot Verify

No test file changes visible in this PR
Critical: Need to verify:
1. Tests exist for both CommonJS and ESM contexts
2. Tests verify the idempotent behavior of importNodeDependencies()
3. Tests check error handling when Node modules aren't available
4. Integration tests verify registry.start() works correctly

Recommendation: Add or update tests to cover:

// Test cases needed:
- importNodeDependencies() in CommonJS context
- importNodeDependencies() in ESM context  
- importNodeDependencies() called multiple times (idempotent)
- Error when neither require nor import.meta.url available
- Driver creation in both sync and async contexts

Additional Concerns

JSON formatting change (asyncapi.json)
- Changed from tabs to 2-space indentation
- Added trailing newline
- Question: Was this intentional or an auto-formatter change? Should verify this doesn't break any tooling.
Removed await without corresponding Promise chain (dump-openapi.ts:23)
```
// Before: await createFileSystemOrMemoryDriver(false)
// After: createFileSystemOrMemoryDriver(false)
```
- Now synchronous, which is correct given the function signature change
- No issue, but verify the dump-openapi script still works
Missing await cleanup (engine-process/mod.ts:29)
- The await was removed from importNodeDependencies() call
- This is correct since the function is now sync
- However, ensureEngineProcess remains async and properly awaits other operations

Recommendations

High Priority:

Add explicit error handling for unsupported module systems in getRequireFn()
Add comprehensive tests for both CommonJS and ESM contexts
Document the deferred initialization behavior in registry.start() JSDoc

Medium Priority:
4. Replace @ts-ignore with @ts-expect-error and add specific error codes
5. Remove redundant warning before throw in error handler (pick one: log or throw)
6. Verify asyncapi.json formatting change is intentional

Low Priority:
7. Add environment validation before attempting require
8. Consider extracting the require/import.meta.url logic into a separate utility

Verdict

✅ Approved with suggestions

The core change (async import → sync require) is sound and well-reasoned. The implementation is mostly solid, but would benefit from:

Better error handling in edge cases
Comprehensive test coverage
Clearer documentation about deferred initialization

The changes align with the goal of making registry.start() synchronous while maintaining backward compatibility.

Overall Assessment:

Code Quality: 8/10
Security: 9/10
Performance: 9/10
Test Coverage: ?/10 (cannot verify from PR)

vercel · 2025-11-13T20:54:06Z

Deployment failed with the following error:

Too many requests - try again in 1 minute (more than 120, code: "api-deployments-flood-pro").

Learn More: https://vercel.com/docs/limits#rate-limits

claude · 2025-11-13T20:56:40Z

Pull Request Review

Summary

This PR converts dynamic import() statements to synchronous require() calls for Node.js module loading in RivetKit. The primary motivation is to make registry.start() synchronous, which is a breaking API change requirement.

Code Quality & Best Practices

✅ Strengths

Well-documented rationale: The comments in node.ts:5-11 clearly explain why require() is needed instead of async import()
Proper error handling: The importNodeDependencies() function includes try-catch with informative error messages
Idempotent design: The hasImportedDependencies flag ensures modules are only loaded once
Cross-environment support: The getRequireFn() function correctly handles both CommonJS and ESM contexts
Webpack compatibility: Uses /* webpackIgnore: true */ comments to prevent bundling issues
Consistent API: All functions that previously returned Promise<DriverConfig> now return DriverConfig synchronously

⚠️ Concerns

TypeScript @ts-ignore usage: Multiple @ts-ignore comments suppress type errors (lines 32, 54-67 in node.ts). While necessary for dynamic require, consider using @ts-expect-error instead, which will error if the suppression becomes unnecessary.
Synchronous blocking: The transition to synchronous module loading means the main thread will block during initial module imports. This is acceptable for startup, but worth documenting in user-facing docs if this impacts startup performance.
Missing null checks: In node.ts:25-34, the function assumes require or import.meta.url exists but doesn't handle edge cases where neither might be available.

Potential Bugs

Error swallowing in catch block: In node.ts:71-77, the error is both logged with console.warn AND re-thrown. The warning message says "will not work" but then throws, which could be confusing. Consider using console.error or just throwing without the warning.
Getter functions don't re-check validity: Functions like getNodeCrypto() only check if the module was loaded but don't verify if it's still valid. If the module somehow becomes undefined after loading, these functions would throw unclear errors.

Performance Considerations

✅ Synchronous loading is appropriate: Since these modules are only needed in Node.js environments and loaded once at startup, synchronous loading is fine and actually preferable for API simplicity.
✅ Lazy evaluation: The modules are only loaded when importNodeDependencies() is called, not on file import, which is good for environments where Node modules aren't needed.

Security Concerns

✅ No security issues identified: The code only loads standard Node.js built-in modules using the node: protocol, which is secure.
Webpack ignore comments: The /* webpackIgnore: true */ comments are correctly placed to prevent accidental bundling of Node.js modules.

Test Coverage

⚠️ Cannot assess test coverage from this PR alone, but recommend:

Testing in both CommonJS and ESM environments
Testing error paths when Node.js modules aren't available (browser/edge runtime)
Verifying hasImportedDependencies flag prevents re-imports

Additional Observations

JSON formatting change: The asyncapi.json file changed from tabs to spaces. This appears to be an auto-formatter change and doesn't affect functionality, but is significant in line count.
Missing implementation in dump-openapi.ts: Added listActors: unimplemented on line 41, which appears to be a required addition for the new API but is marked as unimplemented. This should be tracked separately if it's meant to be implemented.
Removed await keywords: Several await keywords removed from function calls that are now synchronous (e.g., registry/mod.ts:138). All changes appear correct and consistent.

Recommendations

Replace @ts-ignore with @ts-expect-error for better type safety
Add explicit error handling for edge cases in getRequireFn()
Consider using console.error instead of console.warn in the catch block since the error is re-thrown
Verify test coverage for both CommonJS and ESM module systems
Document the synchronous nature of registry.start() in user-facing documentation if this is a breaking change

Verdict

✅ Approve with minor suggestions

The PR accomplishes its goal of making module loading synchronous to support a synchronous registry.start() API. The implementation is well-structured with proper cross-environment support. The suggested improvements are minor and mostly related to TypeScript usage and error messaging clarity.

graphite-app · 2025-11-13T21:34:17Z

Merge activity

Nov 13, 9:34 PM UTC: NathanFlurry added this pull request to the Graphite merge queue.
Nov 13, 9:34 PM UTC: The Graphite merge queue removed this pull request due to downstack failures on PR #3441.
Nov 13, 9:37 PM UTC: The Graphite merge queue removed this pull request due to downstack failures on PR #3441.
Nov 13, 9:37 PM UTC: NathanFlurry added this pull request to the Graphite merge queue.
Nov 13, 9:43 PM UTC: NathanFlurry added this pull request to the Graphite merge queue.
Nov 13, 9:43 PM UTC: The Graphite merge queue removed this pull request due to downstack failures on PR #3441.
Nov 13, 10:04 PM UTC: NathanFlurry added this pull request to the Graphite merge queue.
Nov 13, 10:05 PM UTC: The Graphite merge queue removed this pull request due to downstack failures on PR #3441.
Nov 13, 10:11 PM UTC: NathanFlurry added this pull request to the Graphite merge queue.
Nov 13, 10:12 PM UTC: The Graphite merge queue removed this pull request due to downstack failures on PR #3442.

claude · 2025-11-13T21:42:05Z

Pull Request Review

Summary

This PR switches from async dynamic imports to synchronous require calls for Node.js module loading in RivetKit. The primary motivation is to enable registry.start() to remain synchronous while still accessing Node.js modules.

Positive Aspects

Clear Motivation: The comments explain the reasoning well - registry.start() needs to be synchronous for API compatibility
Idempotent Design: The importNodeDependencies() function uses a flag to ensure it only runs once
Error Handling: Proper error handling with informative messages when modules are not available
Webpack Compatibility: Uses webpackIgnore comments to prevent bundling issues
ESM and CommonJS Support: The getRequireFn() helper handles both module systems gracefully
Consistent Changes: All async function signatures properly updated to synchronous

Concerns and Recommendations

1. Synchronous I/O in Module Loading (Medium Priority)

The switch from async to sync means require() will block the event loop when loading modules. While this happens only once at startup, it could impact cold start times in serverless environments.

Recommendation: Consider if there is a way to preserve async initialization through a lazy loading pattern or factory function that returns a promise.

2. Type Safety Issues (Low Priority)

Multiple @ts-ignore comments are used for dynamic require calls. Consider using @ts-expect-error with specific error codes or creating proper type assertions.

3. JSON Formatting Change (Minor)

The asyncapi.json file shows a large diff due to formatting changes. While the actual content appears unchanged, this makes the diff harder to review.

Recommendation: Consider separating formatting changes into a separate commit or PR to keep the functional changes clearer.

4. Missing Test Coverage (Medium Priority)

The changes affect critical initialization paths but I do not see new tests verifying both ESM and CommonJS contexts work correctly, error handling when Node.js modules are not available, and idempotent behavior of importNodeDependencies().

Recommendation: Add tests for the new synchronous loading behavior, especially edge cases.

5. Error Message Clarity (Low Priority)

Line 72-76 in node.ts uses console.warn before throwing which might create duplicate error messages. Also uses console.warn instead of the structured logging used elsewhere.

Recommendation: Use structured logging and include the original error as the cause.

6. Potential Race Condition (Low Priority)

The hasImportedDependencies flag check at line 46 is not atomic. If multiple callers invoke importNodeDependencies() concurrently before the first completes they could all attempt to load modules.

Recommendation: While unlikely to cause issues in practice since this runs at startup, consider using a promise-based locking mechanism if concurrent calls are possible.

Security Considerations

No security concerns identified. The change reduces async complexity which could actually reduce certain timing-based vulnerabilities.

Performance Considerations

Cold starts: Synchronous module loading may slightly increase cold start times in serverless environments. Runtime: No runtime impact after initialization. Overall: The performance impact should be negligible for most use cases.

Code Quality

The code follows good practices with descriptive function names, clear comments explaining the design decisions, proper error handling with helpful messages, and consistent patterns across files.

Verdict

Approve with minor suggestions. The core change is sound and addresses a real API constraint. The recommendations above would improve the robustness and maintainability but none are blocking issues.

claude · 2025-11-13T21:44:51Z

PR Review: Switch Dynamic Node Imports to Use Require

Summary

This PR refactors the RivetKit TypeScript codebase to replace async import() statements with synchronous require() for Node.js module loading. The main motivation is to allow registry.start() to remain synchronous while still dynamically loading Node.js modules only when needed.

Code Quality & Best Practices

Strengths

Well-documented rationale: The comments in node.ts clearly explain why require() is used instead of async import():
- Lines 5-10 provide excellent context
- The reasoning is sound for maintaining a synchronous API
Idempotent implementation: The importNodeDependencies() function correctly implements the idempotent pattern with hasImportedDependencies flag (line 46)
Cross-environment compatibility: Good handling of both CommonJS and ESM contexts via getRequireFn() (lines 25-34)
Consistent API: All function signatures correctly updated from async to synchronous across:
- chooseDefaultDriver()
- createFileSystemOrMemoryDriver()
- createFileSystemDriver()
- createMemoryDriver()
- registry.start()
Webpack compatibility: Uses webpackIgnore comments to prevent bundling (lines 54-68)

Issues & Concerns

Test file not updated (driver-file-system.test.ts:13):
The test still uses await even though createFileSystemOrMemoryDriver() is now synchronous. This will work but is misleading and should be removed.
Error handling concern (node.ts:71-77):
- console.warn() is used instead of the project's logging system
- The error is logged then immediately re-thrown, making the console.warn redundant
- Consider using the tracing/logging infrastructure from the project or removing the redundant log
TypeScript @ts-ignore usage:
- Multiple @ts-ignore comments (lines 32, 53-67) suppress type checking
- While necessary for dynamic requires, consider adding specific disable comments like @ts-expect-error with explanations for better type safety
Missing type assertion (node.ts:50):
- getRequireFn() returns NodeRequire | ((id: string) => any) but the type isn't explicitly handled
- Consider adding explicit return type annotation

Potential Bugs

Synchronous blocking: The switch from async to sync means Node.js module loading now blocks the event loop. While this is intentional, it could cause issues in high-concurrency scenarios at startup. This seems acceptable given the early initialization context.
Missing dump-openapi.ts update (dump-openapi.ts:23):
The file adds a new listActors: unimplemented, field but doesn't remove the await from line 23.

Performance Considerations

Positive impact:

Synchronous loading simplifies the initialization flow
No runtime performance difference once modules are loaded
Maintains lazy loading (only loads when needed)

Startup consideration:

First call to importNodeDependencies() will block briefly while loading modules
This is acceptable for initialization but worth documenting

Security Concerns

No major security issues identified

Proper use of node: protocol for built-in modules
No external dependencies introduced
Webpack ignore comments are appropriate

Test Coverage

Test gaps:

No tests verify the new synchronous behavior
No tests verify the idempotent loading (calling importNodeDependencies() multiple times)
No tests verify proper error handling when Node.js modules aren't available (e.g., browser environment)

Recommendations:

Add unit test for importNodeDependencies() idempotency
Add test for error handling in non-Node environments
Update existing test to remove unnecessary await

AsyncAPI JSON Changes

The asyncapi.json changes appear to be formatting-only (tabs to spaces, array formatting). This is likely a side effect of running a formatter. No functional changes detected.

Action Items

Required before merge:

Remove await in driver-file-system.test.ts:13
Remove await in dump-openapi.ts:23
Consider: Improve error handling in importNodeDependencies() (use proper logger or remove redundant warn)

Nice to have:
4. Add tests for the new synchronous loading behavior
5. Replace @ts-ignore with @ts-expect-error with explanations
6. Add explicit return type to getRequireFn()

Overall Assessment

This is a solid refactoring with clear motivation and good implementation. The main issues are:

Two await keywords that need removal (test file and dump-openapi)
Minor code quality improvements around error handling and TypeScript strictness

The architectural decision to use synchronous require is well-justified and properly implemented. Once the await keywords are removed, this should be ready to merge.

Recommendation: Request changes for the two await removals, then approve.

claude · 2025-11-13T22:31:44Z

Code Review: Switch Dynamic Node Imports to Use Require

Overview

This PR refactors the RivetKit TypeScript package to use synchronous require() instead of async import() for Node.js module loading. The main motivation is that registry.start() cannot be async and needs immediate access to Node modules during setup.

Strengths

✅ Well-architected centralization: The new src/utils/node.ts module provides a clean single source of truth for Node.js dependencies with proper encapsulation.

✅ Excellent linting enforcement: The biome.json configuration cleverly restricts Node.js imports to only the utils/node.ts file, preventing future violations of this pattern.

✅ Comprehensive documentation: The inline comments clearly explain the rationale for using require() over import().

✅ Proper error handling: The importNodeDependencies() function includes appropriate try-catch with informative error messages.

✅ Cross-environment compatibility: The getRequireFn() helper correctly handles both CommonJS and ESM contexts.

Issues & Concerns

🔴 Critical: Removed Node.js Import from dump-openapi.ts

Location: rivetkit-typescript/packages/rivetkit/scripts/dump-openapi.ts:1-2

The script still uses direct Node.js imports:

import * as fs from "node:fs/promises";
import { resolve } from "node:path";

Issue: These should use the new helper functions from @/utils/node for consistency.

Impact:

Inconsistent with the stated goal of centralizing Node.js imports
The script is in the scripts/ directory, not src/, so it's excluded from the biome linting rules

Recommendation: Either:

Update the script to use getNodeFs() and getNodePath() (preferred for consistency)
Add scripts/**/* to the biome override exclusions if scripts are intentionally exempt

🟡 Medium: Webpack Ignore Comments Effectiveness

Location: src/utils/node.ts:54-68

// @ts-ignore
nodeCrypto = requireFn(/* webpackIgnore: true */ "node:crypto");

Issue: The /* webpackIgnore: true */ comments are for webpack's dynamic import handling, but they may not work as intended with require() calls. Different bundlers handle magic comments differently.

Recommendation:

Test that bundling actually excludes these Node.js modules
Consider documenting which bundlers this has been tested with
May need additional configuration in bundler configs (webpack.config.js, vite.config.ts, etc.)

🟡 Medium: Synchronous Initialization Side Effects

Location: src/drivers/file-system/global-state.ts:96-136

The constructor now calls importNodeDependencies() indirectly, but the actual call is in createFileSystemOrMemoryDriver() at line 16 of src/drivers/file-system/mod.ts.

Issue: There's potential for race conditions if:

FileSystemGlobalState is instantiated before importNodeDependencies() is called
Multiple parts of the code try to access Node.js modules before initialization

Recommendation:

Add defensive checks in getter functions to ensure modules are loaded
Consider calling importNodeDependencies() at the top of FileSystemGlobalState constructor for safety
Document the initialization order requirements

🟡 Medium: Missing Assert Import Replacement

Location: engine/sdks/typescript/runner-protocol/src/index.ts:1

-import assert from "node:assert"

Issue: The assert import was removed and replaced with a custom implementation at the bottom of the file. While this works, it:

Changes behavior (Node.js assert has more features)
Is undocumented why this specific change was needed
Custom assert is less robust (no equality checks, etc.)

Recommendation:

If this is just for the assert(condition, message) signature, that's fine
Add a comment explaining why Node.js assert was removed here (likely because this is a protocol SDK that should work in browsers)
Consider if other assertion sites in the file need updating

🔵 Low: TypeScript @ts-ignore Usage

Location: Multiple locations in src/utils/node.ts

Issue: Multiple @ts-ignore comments are used instead of more specific @ts-expect-error.

Recommendation: Use @ts-expect-error with explanatory comments:

// Dynamic require is intentional; types don't recognize this pattern
// @ts-expect-error - requireFn returns any for dynamic module loading
nodeCrypto = requireFn("node:crypto");

This provides better type safety since @ts-expect-error will error if the line no longer has a type issue.

🔵 Low: engine/package.json Simplification

Location: engine/package.json

Issue: The package.json was dramatically simplified, removing scripts, devDependencies, and resolutions. This seems like a significant change beyond the scope of switching to require().

Questions:

Is this intentional cleanup?
Are these scripts/deps no longer needed?
Should this be a separate commit/PR for clarity?

Recommendation: If intentional, add a note in the PR description explaining this cleanup.

🔵 Low: AsyncAPI JSON Formatting Only

Location: rivetkit-asyncapi/asyncapi.json

Issue: This file shows only formatting changes (tabs to spaces), which adds noise to the diff.

Recommendation:

If auto-generated, that's fine - note it in the PR description
If manual, consider separating formatting changes from functional changes

Testing Concerns

⚠️ Missing explicit test coverage: The PR doesn't add new tests for the dynamic loading mechanism. Consider adding:

Unit tests for src/utils/node.ts:
- Test importNodeDependencies() is idempotent
- Test getter functions throw when modules not loaded
- Test both CommonJS and ESM contexts
Integration tests:
- Verify file system driver works with dynamic imports
- Test initialization order scenarios

The existing test files only show minor import path changes, not functional verification of the new loading mechanism.

Performance Considerations

✅ Good: Using require() for synchronous loading is appropriate here since these modules are only needed in Node.js environments and the setup must be synchronous.

✅ Good: The idempotent check (hasImportedDependencies) prevents redundant loading.

⚠️ Note: All Node.js modules are loaded upfront even if not all are used. This is fine for a small set of core modules but could be optimized to lazy-load if the list grows significantly.

Security Considerations

✅ No security issues identified: The dynamic requires are for built-in Node.js modules only, not user-controlled paths.

Recommendations Summary

Must Fix Before Merge:

Update dump-openapi.ts to use the new pattern or explicitly exclude from linting rules

Should Fix Before Merge:
2. Add defensive initialization checks in FileSystemGlobalState constructor
3. Document the webpack ignore comment testing/bundler compatibility
4. Clarify the engine/package.json simplification

Nice to Have:
5. Add unit tests for the new dynamic loading utilities
6. Replace @ts-ignore with @ts-expect-error + comments
7. Document why custom assert was needed in runner-protocol

Overall Assessment

Status: ⚠️ Needs Minor Revisions

The approach is sound and the implementation is generally well-done. The centralization of Node.js imports through src/utils/node.ts with linting enforcement is a good architectural pattern. However, there are a few inconsistencies (mainly the dump-openapi.ts script) and missing test coverage that should be addressed before merging.

The change successfully achieves its goal of making registry.start() synchronous while maintaining clean code organization.

claude · 2025-11-13T22:34:48Z

Code Review

This PR switches from async dynamic imports to synchronous require() for Node.js modules. While the core change addresses the API constraint that registry.start() needs to be synchronous, there are several concerns:

Critical Issues

1. Breaking Test Code

The tests in tests/driver-file-system.test.ts:13-17 and tests/driver-memory.test.ts:17 still use await createFileSystemOrMemoryDriver() even though these functions are now synchronous. This will cause incorrect behavior.

2. Error Handling Inconsistency (src/utils/node.ts:71-77)

The error handling in importNodeDependencies() both logs a warning AND throws the error. This is redundant since throwing already signals the error. Recommend removing the console.warn.

Code Quality Issues

3. Synchronous Module Loading Trade-offs

The switch from async import() to synchronous require() has implications:

Pros: Allows registry.start() to remain synchronous
Cons: Blocks event loop during module loading, less aligned with ESM practices

The comments explain the reasoning, but consider if the API could support async initialization.

4. Type Safety with @ts-ignore

Multiple consecutive @ts-ignore directives reduce type safety. Consider consolidating or adding specific explanations for each.

Minor Issues

5. AsyncAPI JSON Formatting

The massive diff in asyncapi.json is just formatting (tabs to spaces). This should ideally be in a separate commit or the file should be excluded if auto-generated.

6. Missing Error Handling in getRequireFn()

The function assumes if require is not available, we're in ESM. Add explicit error handling if both contexts fail.

Recommendations

Fix the tests - Remove await from synchronous driver creation calls
Clean up error handling - Remove redundant console.warn
Add integration tests for both CommonJS and ESM contexts
Separate formatting changes from functional changes
Verify all tests pass after the async-to-sync changes

The approach is pragmatic but needs cleanup before merging.

claude · 2025-11-13T22:35:29Z

PR Review: Switch dynamic node imports to use require

Summary

This PR refactors the Node.js module loading strategy in RivetKit from async import() to synchronous require(). The change is driven by the need to make registry.start() synchronous, which requires immediate access to Node.js modules during setup.

Code Quality & Best Practices

Positive aspects:

✅ Well-documented rationale for using require() over import() in comments (node.ts:5-10)
✅ Idempotent design with hasImportedDependencies flag prevents multiple imports
✅ Proper error handling and informative error messages
✅ Webpack ignore comments prevent bundling issues
✅ Consistent pattern across all affected files

Issues & Recommendations:

Breaking API change - The PR changes the signature of several exported functions from async to sync:
```
// Before
export async function createFileSystemDriver(opts?: { path?: string }): Promise<DriverConfig>

// After  
export function createFileSystemDriver(opts?: { path?: string }): DriverConfig
```
Impact: This is a breaking change for existing code that uses await with these functions. Consider:
- Documenting this in a CHANGELOG or migration guide
- Checking if any internal code still uses await with these functions
Test compatibility issue - The tests at driver-file-system.test.ts:13 and driver-memory.test.ts:17 still use await with the now-synchronous functions:
```
// This will still work but the await is now unnecessary
driver: await createFileSystemOrMemoryDriver(false)
```
Recommendation: Update the tests to remove unnecessary await keywords for clarity.

Error handling could be improved - In node.ts:44-78, the function catches errors and both logs them with console.warn AND re-throws them. This could lead to duplicate error handling by callers:

} catch (err) {
    console.warn(
        "Node.js modules not available, file system driver will not work",
        err,
    );
    throw err; // Re-throwing after logging
}

Recommendation: Consider either logging OR throwing, not both. If you want to provide context, wrap in a new error:

} catch (err) {
    throw new Error(
        `Node.js modules not available, file system driver will not work: ${err}`,
    );
}

Potential Bugs

TypeScript @ts-ignore directives - Multiple @ts-ignore comments are used (lines 32, 53-67 in node.ts) which suppress type checking. While this may be necessary for dynamic requires, it increases the risk of runtime errors.

Recommendation: Consider using @ts-expect-error instead, which will error if the suppression becomes unnecessary in the future.
Missing error handling in registry.start() - The importNodeDependencies() call in drivers/file-system/mod.ts:16 could throw, but there's no try-catch. If Node modules aren't available, the error will bubble up with a generic message.

Recommendation: Consider wrapping this in a try-catch to provide context about which driver is failing:
```
try {
    importNodeDependencies();
} catch (err) {
    throw new Error(
        `Failed to initialize ${persist ? 'file-system' : 'memory'} driver: ${err}`
    );
}
```

Performance Considerations

✅ Positive impact: The change from async to sync should slightly improve performance by:

Eliminating promise overhead
Reducing microtask queue usage
Making startup more predictable and faster

⚠️ Note: The require() calls are only executed once due to the idempotency guard, so there's no performance concern about repeated loading.

Security Concerns

No significant security issues identified. The webpack ignore comments are appropriate to prevent bundling Node.js modules for browser environments.

Test Coverage

Issues identified:

Tests need updating - As mentioned above, tests still use await with now-synchronous functions. While this doesn't break functionality (awaiting a non-promise just returns the value), it's misleading.
Missing test for require() fallback - The code has logic to handle both CommonJS and ESM contexts (node.ts:25-34), but I don't see tests verifying both paths work correctly.

Recommendation: Add tests that verify:
- CommonJS context uses global require
- ESM context uses createRequire(import.meta.url)
- Error handling when Node modules aren't available
dump-openapi.ts uses old pattern - The script at scripts/dump-openapi.ts:23 has been updated but there's an unrelated unimplemented function on line 41 that might need attention in a separate PR.

AsyncAPI JSON Changes

The changes to rivetkit-asyncapi/asyncapi.json appear to be pure formatting (tabs to spaces). This is fine but:

Recommendation: Ensure your linter/formatter is configured consistently to avoid these changes in future PRs. Consider adding a .editorconfig file if not already present.

Overall Assessment

This is a well-executed refactoring with clear motivation. The main concerns are:

Breaking change - Document this for users
Test updates needed - Remove unnecessary awaits
Error handling improvements - Avoid double-handling errors
Additional test coverage - Test ESM/CommonJS paths

The changes are backward compatible in practice (awaiting a non-promise works), but should be cleaned up for clarity and future maintainability.

Recommendation: ✅ Approve with minor changes suggested

vercel bot had a problem deploying to Preview – rivetkit-serverless November 13, 2025 03:24 Failure

This was referenced Nov 13, 2025

chore(site): fix generating examples #3453

Open

fix(rivetkit): fix onError handler not working #3455

Merged

NicholasKissel mentioned this pull request Nov 13, 2025

chore: create branch for v3 website changes on v2 #3304

Open

NathanFlurry mentioned this pull request Nov 13, 2025

chore(rivetkit): fix type checks #3462

Merged

This was referenced Nov 13, 2025

feat(gas): implement smart pulling for horizontally scaled workers #3417

Merged

fix(guard): handle websocket tasks during shutdown #3448

Merged

fix: improve sigterm handling for the entire runtime #3443

Merged

NathanFlurry mentioned this pull request Nov 13, 2025

feat(gateway): websocket hibernation #3400

Merged

This was referenced Nov 13, 2025

fix(pb): dont immediately reallocate actors on runner stopping #3413

Merged

fix(guard): dont send open msg for hibernating ws, hibernating ws keepalive #3449

Merged

NathanFlurry mentioned this pull request Nov 13, 2025

chore(gas): rename wake sub to bump sub #3408

Merged

jog1t mentioned this pull request Nov 13, 2025

refactor: performance improvements for inspector #3452

Draft

MasterPtato approved these changes Nov 13, 2025

View reviewed changes

NathanFlurry force-pushed the rivetkit_remove_dependency_node_modules branch from 12726ac to 7bda860 Compare November 13, 2025 20:53

NathanFlurry force-pushed the 11-12-chore_rivetkit_switch_dynamic_node_imports_to_use_require branch from 4a72d89 to 0ceae4a Compare November 13, 2025 20:54

NathanFlurry mentioned this pull request Nov 13, 2025

chore(rivetkit): implement new hibernating ws protocol #3464

Open

vercel bot had a problem deploying to Preview – rivetkit-serverless November 13, 2025 21:10 Failure

NathanFlurry force-pushed the 11-12-chore_rivetkit_switch_dynamic_node_imports_to_use_require branch from 0ceae4a to c1f60cd Compare November 13, 2025 21:39

NathanFlurry force-pushed the rivetkit_remove_dependency_node_modules branch from 7bda860 to 981bd59 Compare November 13, 2025 21:39

vercel bot had a problem deploying to Preview – rivetkit-serverless November 13, 2025 21:40 Failure

NathanFlurry mentioned this pull request Nov 13, 2025

feat(cloudflare-workers): add createInlineClient #3466

Open

NathanFlurry force-pushed the rivetkit_remove_dependency_node_modules branch from 981bd59 to ec6a9cb Compare November 13, 2025 22:27

NathanFlurry force-pushed the 11-12-chore_rivetkit_switch_dynamic_node_imports_to_use_require branch from c1f60cd to 3ece7ec Compare November 13, 2025 22:27

vercel bot had a problem deploying to Preview – rivetkit-serverless November 13, 2025 22:28 Failure

Base automatically changed from rivetkit_remove_dependency_node_modules to main November 13, 2025 22:29

chore(rivetkit): switch dynamic node imports to use require

b75e10f

NathanFlurry force-pushed the 11-12-chore_rivetkit_switch_dynamic_node_imports_to_use_require branch from 3ece7ec to b75e10f Compare November 13, 2025 22:30

NathanFlurry merged commit 90b37fa into main Nov 13, 2025
6 of 10 checks passed

NathanFlurry deleted the 11-12-chore_rivetkit_switch_dynamic_node_imports_to_use_require branch November 13, 2025 22:31

vercel bot had a problem deploying to Preview – rivetkit-serverless November 13, 2025 22:31 Failure

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

chore(rivetkit): switch dynamic node imports to use require #3461

chore(rivetkit): switch dynamic node imports to use require #3461

Uh oh!

NathanFlurry commented Nov 13, 2025

Uh oh!

vercel bot commented Nov 13, 2025 •

edited

Loading

Uh oh!

NathanFlurry commented Nov 13, 2025 •

edited

Loading

Uh oh!

claude bot commented Nov 13, 2025

Uh oh!

vercel bot commented Nov 13, 2025

Uh oh!

claude bot commented Nov 13, 2025

Uh oh!

graphite-app bot commented Nov 13, 2025 •

edited

Loading

Uh oh!

claude bot commented Nov 13, 2025

Uh oh!

claude bot commented Nov 13, 2025

Uh oh!

Uh oh!

claude bot commented Nov 13, 2025

Uh oh!

claude bot commented Nov 13, 2025

Uh oh!

claude bot commented Nov 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

chore(rivetkit): switch dynamic node imports to use require #3461

chore(rivetkit): switch dynamic node imports to use require #3461

Uh oh!

Conversation

NathanFlurry commented Nov 13, 2025

Uh oh!

vercel bot commented Nov 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NathanFlurry commented Nov 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

How to use the Graphite Merge Queue

Uh oh!

claude bot commented Nov 13, 2025

Pull Request Review

Summary

Code Quality & Best Practices

✅ Strengths

⚠️ Potential Issues

Performance Considerations

✅ Positive

⚠️ Considerations

Security Concerns

✅ No Major Issues

💡 Suggestions

Test Coverage

⚠️ Cannot Verify

Additional Concerns

Recommendations

Verdict

Uh oh!

vercel bot commented Nov 13, 2025

Uh oh!

claude bot commented Nov 13, 2025

Pull Request Review

Summary

Code Quality & Best Practices

✅ Strengths

⚠️ Concerns

Potential Bugs

Performance Considerations

Security Concerns

Test Coverage

Additional Observations

Recommendations

Verdict

Uh oh!

graphite-app bot commented Nov 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge activity

Uh oh!

claude bot commented Nov 13, 2025

Pull Request Review

Summary

Positive Aspects

Concerns and Recommendations

1. Synchronous I/O in Module Loading (Medium Priority)

2. Type Safety Issues (Low Priority)

3. JSON Formatting Change (Minor)

4. Missing Test Coverage (Medium Priority)

5. Error Message Clarity (Low Priority)

6. Potential Race Condition (Low Priority)

Security Considerations

Performance Considerations

Code Quality

Verdict

Uh oh!

claude bot commented Nov 13, 2025

PR Review: Switch Dynamic Node Imports to Use Require

Summary

Code Quality & Best Practices

Strengths

Issues & Concerns

Potential Bugs

Performance Considerations

Security Concerns

Test Coverage

AsyncAPI JSON Changes

Action Items

Overall Assessment

vercel bot commented Nov 13, 2025 •

edited

Loading

NathanFlurry commented Nov 13, 2025 •

edited

Loading

graphite-app bot commented Nov 13, 2025 •

edited

Loading