Fix macro resolve ordering when macros depend on other macros#1756
Fix macro resolve ordering when macros depend on other macros#1756MarcelOlsen wants to merge 4 commits intoelysiajs:mainfrom
Conversation
|
@coderabbitai review |
WalkthroughAdds discriminated macro typing and MacroContext propagation, refactors runtime macro expansion in src/index.ts to a two-pass cached-entries algorithm, exposes a new public type DiscriminatedMacroEntry, and adds extensive tests validating macro resolve dependency ordering and cross-plugin resolution scenarios. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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. Comment Tip CodeRabbit can use your project's `biome` configuration to improve the quality of JS/TS/CSS/JSON code reviews.Add a configuration file to your project to customize how CodeRabbit runs |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/macro/macro.test.ts`:
- Around line 1593-1625: The test "deduplication preserved with shared
dependencies" records baseCallCount but never asserts it; update the test inside
the same it block (the one that builds the Elysia instance with .macro and calls
app.handle) to assert that baseCallCount equals 1 after awaiting the response
JSON so the test explicitly verifies that the shared dependency resolver (the
base resolve function) was executed only once.
- Line 1481: The test contains a no-op assertion because it references a
misspelled field `hasessions` and does not call the matcher; update the
assertion on the `response` object to use the correct property name
(`hasSessions`) and invoke the matcher method (e.g., change the call to
expect(response.hasSessions).not.toBeUndefined() or another appropriate matcher
like toBeTruthy()) so the test actually validates the presence of the
cross-plugin dependency.
e5b69e6 to
d139a15
Compare
commit: |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
resolves: #1743
Problem
When a macro's resolve function depends on context provided by another macro's resolve, the dependent macro's resolve runs before the dependency. This results in the dependency's values being undefined at runtime.
Root Cause
In
applyMacro(), the inner loop iterates overObject.entries(macroHook)in JavaScript insertion order. Forauth: { resolve: fn, sessions: true }:k = 'resolve'-auth.resolveis appended tolocalHookfirst.k = 'sessions'- recursive expansion addssessions.resolveafterward.Result:
localHook.resolve = [auth.resolve, sessions.resolve]- incorrect order.Fix
Split the single inner loop into two passes:
This produces
localHook.resolve = [sessions.resolve, auth.resolve]regardless of property declaration order.Summary by CodeRabbit
New Features
Refactor
Tests