fix(plugin): faster hashing#7338
Conversation
github-actions
Bot
added
documentation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughReplaces manual crypto.createHash(...).update(...).digest(...) usage with a new exported hash(algorithm, data, encoding) helper that prefers Node’s crypto.hash API; callers in start-plugin-core and router-generator plus docs/examples and the changeset are updated to use it. ChangesHash API Modernization
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
View your CI Pipeline Execution ↗ for commit 970a957
☁️ Nx Cloud last updated this comment at |
🚀 Changeset Version Preview3 package(s) bumped directly, 7 bumped as dependents. 🟩 Patch bumps
|
Bundle Size Benchmarks
Current gzip tracks all emitted client JS chunks. Initial gzip tracks only the entry/import graph. Trend sparkline is historical current gzip ending with this PR measurement; lower is better. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/start/framework/react/guide/server-functions.md`:
- Line 382: The example calls crypto.hash in the snippet but never imports
crypto; add an explicit import for Node's crypto module at the top of the
example (e.g. import or require the 'crypto' module) so the return statement
using crypto.hash(`${filename}--${functionName}`,'hex') is runnable; update the
snippet surrounding the function that returns crypto.hash(...) to include that
import.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 121c8622-6a8f-47eb-9615-e026d715d414
📒 Files selected for processing (5)
.changeset/many-ads-smile.mddocs/start/framework/react/guide/isr.mddocs/start/framework/react/guide/server-functions.mdpackages/router-generator/src/generator.tspackages/start-plugin-core/src/start-compiler/compiler.ts
Merging this PR will not alter performance
Comparing 
Footnotes
|
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
At least one additional CI pipeline execution has run since the conclusion below was written and it may no longer be applicable.
Nx Cloud is proposing a fix for your failed CI:
We rename the local exports Map variable in extractModuleInfo to namedExports to fix the five import/no-commonjs ESLint errors introduced when the PR removed the blanket /* eslint-disable import/no-commonjs */ comment from compiler.ts. The rule was incorrectly flagging exports.set(...) calls as CommonJS patterns because the identifier exports is reserved by the rule, even though the variable is a plain local Map<string, string>.
Tip
✅ We verified this fix by re-running @tanstack/start-plugin-core:test:eslint, tanstack-router-e2e-vue-basepath-file-based:test:e2e.
Suggested Fix changes
diff --git a/packages/start-plugin-core/src/start-compiler/compiler.ts b/packages/start-plugin-core/src/start-compiler/compiler.ts
index ed75e5be..f86a1b84 100644
--- a/packages/start-plugin-core/src/start-compiler/compiler.ts
+++ b/packages/start-plugin-core/src/start-compiler/compiler.ts
@@ -764,7 +764,7 @@ export class StartCompiler {
id: string,
): ModuleInfo {
const bindings = new Map<string, Binding>()
- const exports = new Map<string, string>()
+ const namedExports = new Map<string, string>()
const reExportAllSources: Array<string> = []
// we are only interested in top-level bindings, hence we don't traverse the AST
@@ -807,7 +807,7 @@ export class StartCompiler {
if (t.isVariableDeclaration(node.declaration)) {
for (const d of node.declaration.declarations) {
if (t.isIdentifier(d.id)) {
- exports.set(d.id.name, d.id.name)
+ namedExports.set(d.id.name, d.id.name)
bindings.set(d.id.name, { type: 'var', init: d.init ?? null })
}
}
@@ -815,7 +815,7 @@ export class StartCompiler {
}
for (const sp of node.specifiers) {
if (t.isExportNamespaceSpecifier(sp)) {
- exports.set(sp.exported.name, sp.exported.name)
+ namedExports.set(sp.exported.name, sp.exported.name)
}
// export { local as exported }
else if (t.isExportSpecifier(sp)) {
@@ -823,7 +823,7 @@ export class StartCompiler {
const exported = t.isIdentifier(sp.exported)
? sp.exported.name
: sp.exported.value
- exports.set(exported, local)
+ namedExports.set(exported, local)
// When re-exporting from another module (export { foo } from './module'),
// create an import binding so the server function can be resolved
@@ -839,11 +839,11 @@ export class StartCompiler {
} else if (t.isExportDefaultDeclaration(node)) {
const d = node.declaration
if (t.isIdentifier(d)) {
- exports.set('default', d.name)
+ namedExports.set('default', d.name)
} else {
const synth = '__default_export__'
bindings.set(synth, { type: 'var', init: d as t.Expression })
- exports.set('default', synth)
+ namedExports.set('default', synth)
}
} else if (t.isExportAllDeclaration(node)) {
// Handle `export * from './module'` syntax
@@ -855,7 +855,7 @@ export class StartCompiler {
const info: ModuleInfo = {
id,
bindings,
- exports,
+ exports: namedExports,
reExportAllSources,
}
this.moduleCache.set(id, info)
Or Apply changes locally with:
npx nx-cloud apply-locally 69ER-lBKK
Apply fix locally with your editor ↗ View interactive diff ↗
🎓 Learn more about Self-Healing CI on nx.dev
| } | ||
| if (!functionId) { | ||
| functionId = crypto.createHash('sha256').update(entryId).digest('hex') | ||
| functionId = hash('sha256', entryId, 'hex') |
There was a problem hiding this comment.
Since we're on a roll with modernizing things, what do you think about functionId ??= hash('sha256', entryId, 'hex')? I have a personal vendetta against the ! operator it looks like a tiny person shouting at my code 😂
2 participants
Yagiz said it was faster, let's see.
crypto.hash(algorithm, data[, options])nodejs docs
Added in: v21.7.0, v20.12.0
History: v25.4.0 | This API is no longer experimental.
Benchmarks
Isolation micro-benchmark (just comparing node native modules)
Full benchmark (running full plugin with mocked filesystem)
Summary by CodeRabbit