Skip to content

fix: snapshot hook arrays at route registration to preserve order#1832

Open
eL1fe wants to merge 1 commit intoelysiajs:mainfrom
eL1fe:fix/hook-trigger-order
Open

fix: snapshot hook arrays at route registration to preserve order#1832
eL1fe wants to merge 1 commit intoelysiajs:mainfrom
eL1fe:fix/hook-trigger-order

Conversation

@eL1fe
Copy link
Copy Markdown

@eL1fe eL1fe commented Mar 30, 2026

Summary

Routes picked up hooks registered after them due to shared array references.

Problem

new Elysia()
  .get('/a', () => 'A')
  .onBeforeHandle(() => console.log('1'))
  .get('/b', () => 'B')              // should only have hook 1
  .onBeforeHandle(() => console.log('2'))
  .get('/c', () => 'C')              // should have hooks 1, 2

Request to /b triggered both hooks 1 and 2, but should only trigger 1.

Root cause

mergeHook has an early return when the local hook object is empty: return { ...a }. The spread operator copies array properties by reference, not value. So route.hooks.beforeHandle and this.event.beforeHandle pointed to the same array. Later onBeforeHandle calls pushed into that shared array, retroactively adding hooks to already-registered routes.

Changes

  • src/utils.ts: the early return in mergeHook now copies array values to prevent shared references

Test plan

  • /a has 0 hooks, /b has 1, /c has 2
  • Full test suite passes (1525 pass, 0 fail)

Fixes #1826

Summary by CodeRabbit

  • Bug Fixes
    • Improved object merging behavior to prevent shared array references from causing unintended side effects.

Routes registered between onBeforeHandle calls picked up hooks that
were added after them. Root cause: mergeHook's early return path used
shallow spread ({ ...a }) which copies array references, not arrays.
Later pushes to the event arrays mutated already-registered routes.

Now the early return path copies arrays to prevent shared references.

Fixes elysiajs#1826
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 30, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: c2266364-b164-4d16-8f44-ce6d36fec764

📥 Commits

Reviewing files that changed from the base of the PR and between 56310be and c4778db.

📒 Files selected for processing (1)
  • src/utils.ts

Walkthrough

The mergeHook function in src/utils.ts now properly clones array properties by spreading their contents instead of merely spreading the hook object itself, preventing shared array references that were causing incorrect hook execution order~ ♡

Changes

Cohort / File(s) Summary
Hook Merging Logic
src/utils.ts
Modified mergeHook to spread array properties individually when b contains no defined values, ensuring arrays are cloned rather than referenced, fixing hook trigger sequencing~ ♡

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related PRs

Poem

Array refs were sharing~ ♡ *What a mess you made, dummy ヽ(´ー)ノ* *Spread those properties right,* (´;ω;)
Hooks in proper order tonight~ ♡♡♡
How incompetent you've been... (´∀`)♡

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title accurately describes the main change: fixing hook array snapshots to preserve registration order, which directly addresses the core issue of shared array references causing incorrect hook trigger ordering.
Linked Issues check ✅ Passed The code change resolves the linked issue #1826 by fixing the mergeHook function to snapshot array values instead of copying references, ensuring routes only receive hooks registered before them.
Out of Scope Changes check ✅ Passed All modifications in src/utils.ts are directly related to fixing the hook array reference issue described in #1826; no out-of-scope changes detected.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Incorrect hook trigger order.

1 participant