fix: add explicit dependencies syntax for function-syntax named macros (#1574)#1695
fix: add explicit dependencies syntax for function-syntax named macros (#1574)#1695raunak-rpm wants to merge 1 commit intoelysiajs:mainfrom
Conversation
elysiajs#1574) This adds a new type-sound overload for function-syntax named macros that allows users to explicitly declare which previous macros they depend on: `.macro('permission', ['auth'], (perm: string) => ({ auth: true, resolve: ({ user }) => { // 'user' is properly inferred from auth's resolve return { hasPermission: checkPermission(user, perm) } } }))` The explicit dependencies array solves the TypeScript circular inference issue without making unsound assumptions about which macros are enabled. - Adds new overload: macro(name, dependencies, fn) - Dependencies array explicitly lists macro names this macro depends on - MacroContext is computed only from declared dependencies - Existing macro overloads continue to work unchanged Closes elysiajs#1574
WalkthroughIntroduces a new Elysia.macro overload that accepts a named macro with an explicit dependencies array and function syntax, updating TypeScript generics for dependency-aware typing; adds runtime and type-inference tests verifying dependency resolution and backward compatibility. Changes
Sequence Diagram(s)(omitted — change is API/typing addition and tests; no new multi-component runtime sequence requiring visualization) 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
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 |
commit: |
c2528a9 to
9766275
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/macro/runtime-verify-1574.ts`:
- Line 120: The current top-level invocation test().catch(console.error) logs
errors but leaves the process exit code as 0; change the catch to log the error
and exit non‑zero so CI fails on errors (e.g. replace .catch(console.error) with
.catch(err => { console.error(err); process.exit(1); }) to ensure failures from
test() cause a non‑zero exit code).
🧹 Nitpick comments (1)
test/macro/type-inference-1574.ts (1)
104-108: Consider adding a type assertion for empty dependencies test.Test 5 validates the empty dependencies array scenario, but unlike other tests, it doesn't include a type assertion to verify the resolved value type. Adding one would make the test consistent with the others.
Suggested improvement
// Test 5: Empty dependencies array (should still work, just no extra context) const test5 = new Elysia() .macro('standalone', [], (config: { timeout: number }) => ({ - resolve: () => ({ timeout: config.timeout }) + resolve: () => { + const t: number = config.timeout + return { timeout: t } + } }))
test/macro/runtime-verify-1574.ts
Outdated
| process.exit(allPassed ? 0 : 1) | ||
| } | ||
|
|
||
| test().catch(console.error) |
There was a problem hiding this comment.
Error handling may mask failures with exit code 0.
If test() throws an unexpected error, .catch(console.error) logs it but the process exits with code 0 (default), potentially masking CI failures.
Proposed fix
-test().catch(console.error)
+test().catch((err) => {
+ console.error(err)
+ process.exit(1)
+})🤖 Prompt for AI Agents
In `@test/macro/runtime-verify-1574.ts` at line 120, The current top-level
invocation test().catch(console.error) logs errors but leaves the process exit
code as 0; change the catch to log the error and exit non‑zero so CI fails on
errors (e.g. replace .catch(console.error) with .catch(err => {
console.error(err); process.exit(1); }) to ensure failures from test() cause a
non‑zero exit code).
Summary
This PR provides a type-sound solution for issue #1574 - enabling function-syntax named macros to properly infer resolve types from previous macros.
Problem
When using named macros with function syntax, TypeScript couldn't infer the resolve types from previous macros:
Previous Approach (Reverted)
PR #1691 attempted to fix this by computing
MacroContextfrom ALL previous macros, assuming all are enabled. This was reverted because it's not type-sound - at runtime, only the enabled macros would have their resolves available.New Solution: Explicit Dependencies
This PR adds a new overload that allows users to explicitly declare dependencies:
Why This is Type-Sound
auth: true), the runtime behavior matches the typesAPI
Changes
macro<Name, Dependencies, ...>(name, dependencies, fn)insrc/index.tsTest Results
All 1454 tests pass.
Documentation Note
The existing macro overloads continue to work unchanged. The new syntax is opt-in for cases where function-syntax macros need to access previous macro resolves.
Closes #1574
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.