fix(runner-protocol): do not depend on node:assert for generated code #3521
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
3 Skipped Deployments
|
How to use the Graphite Merge QueueAdd 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. |
PR Review: fix(runner-protocol): do not depend on node:assert for generated codeOverviewThis PR removes the dependency on Node.js's ✅ Strengths
🔍 Issues & Recommendations1. String replacement fragility (Minor)Location: The current implementation uses a simple string replacement: let content = content.replace("import assert from \"node:assert\"\n", "");Issue: This assumes the exact format including the newline character. If the BARE compiler changes its formatting (e.g., uses different quotes, adds semicolons, or changes whitespace), this replacement will silently fail. Recommendation: Use a more robust approach with regex or handle multiple variations: // Handle both with and without newline, with/without semicolon
let content = content
.replace("import assert from \"node:assert\";\n", "")
.replace("import assert from \"node:assert\"\n", "")
.replace("import assert from 'node:assert';\n", "")
.replace("import assert from 'node:assert'\n", "");Or use a regex-based approach for more flexibility. 2. String mutation inefficiency (Minor)Location: Issue: The code performs string replacement and then creates a new string with format: let content = content.replace(...); // Creates new String
let content = format\!("{}\n{}", content, assert_function); // Creates another new StringRecommendation: Use a single operation or be more explicit: let mut content = content.replace("import assert from \"node:assert\"\n", "");
content.push_str("\n");
content.push_str(assert_function);3. Formatting inconsistency (Minor)Location: Issue: The custom assert function uses spaces for indentation, but the generated TypeScript likely follows the project's conventions. Looking at the diff output, the generated code doesn't use indentation for top-level functions. Current: function assert(condition: boolean, message?: string): asserts condition {
if (\!condition) throw new Error(message ?? "Assertion failed")
}Recommendation: Match the generated code style (no indentation for function body in this case, or follow the actual generated pattern). 4. Missing error context (Minor)Location: Issue: The Recommendation: let content = fs::read_to_string(file_path)
.unwrap_or_else(|e| panic\!("Failed to read TypeScript file at {:?}: {}", file_path, e));
fs::write(file_path, content)
.unwrap_or_else(|e| panic\!("Failed to write TypeScript file at {:?}: {}", file_path, e));5. No verification of replacement (Medium)Location: Issue: The code doesn't verify that the import was actually found and removed. If the BARE compiler changes and stops emitting the assert import, or if it's already using a different pattern, the build will silently succeed but the custom assert function will be added unnecessarily (and might conflict). Recommendation: Add verification: fn post_process_typescript(file_path: &Path) {
let content = fs::read_to_string(file_path)
.unwrap_or_else(|e| panic\!("Failed to read TypeScript file at {:?}: {}", file_path, e));
// Check if node:assert import exists
let has_node_assert = content.contains("from \"node:assert\"") || content.contains("from 'node:assert'");
if \!has_node_assert {
println\!("cargo:warning=No node:assert import found in generated TypeScript. Post-processing may not be necessary.");
}
// Remove Node.js assert import
let content = content
.replace("import assert from \"node:assert\"\n", "")
.replace("import assert from 'node:assert'\n", "");
// Only add custom assert if we removed the import
let content = if has_node_assert {
format\!("{}\n{}", content, assert_function)
} else {
content
};
fs::write(file_path, content)
.unwrap_or_else(|e| panic\!("Failed to write TypeScript file at {:?}: {}", file_path, e));
}6. Missing test coverage (Medium)Issue: There are no tests verifying that:
Recommendation: Consider adding integration tests or at minimum, a build-time check that the generated file:
🔒 SecurityNo security concerns identified. The code is run during build time and doesn't introduce any runtime vulnerabilities. ⚡ PerformanceNo performance concerns. This is a build-time operation on a single generated file. 📝 Code StyleThe Rust code follows the repository's conventions. The TypeScript formatting should be verified to match generated code patterns. SummaryThis is a good fix for the immediate problem. The main concerns are around robustness (handling format variations) and verification (ensuring the replacement actually happened). Consider implementing the recommendations to make this more maintainable long-term, especially recommendation #5 for verification. Overall Assessment: ✅ Approve with suggestions for improvements |
More templates
@rivetkit/cloudflare-workers
@rivetkit/db
@rivetkit/framework-base
@rivetkit/next-js
@rivetkit/react
rivetkit
@rivetkit/sql-loader
@rivetkit/engine-runner
@rivetkit/engine-runner-protocol
commit: |
jog1t
force-pushed
the
11-21-fix_runner-protocol_do_not_depend_on_node_assert_for_generated_code
branch
from
d899197 to
9e499e8
Compare
|
lgtm :D |
No description provided.