-
-
Notifications
You must be signed in to change notification settings - Fork 385
feat: full path and method for macro introspection metadata #1557
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat: full path and method for macro introspection metadata #1557
Conversation
WalkthroughAdds per-route macro introspection: captures macro options from guards/groups, normalizes and prefixes paths before invoking introspection, propagates route metadata (path, method) through macro application, and exposes MacroIntrospectionMetadata in public types. Tests exercise many introspection scenarios. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant Guard as guard()
participant Group as group()
participant Add as add()
participant Macro as applyMacro()
participant Introspect as introspect hook
Dev->>Guard: .guard({ ...macroOptions })
Guard->>Guard: store guardMacroOptions
Dev->>Group: .group(...)
Group->>Group: set skipMacroIntrospection for child adds
Group->>Group: merge guardMacroOptions for children
Dev->>Add: .add({ path, method })
Add->>Add: normalize path & apply prefix
Add->>Add: merge/restore guardMacroOptions
Add->>Macro: apply macros with route metadata
Macro->>Introspect: introspect(option, { path, method })
Introspect-->>Macro: optional result
Macro-->>Add: expanded macro result
Add-->>Dev: route registered
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
|
Fixes #1498 |
commit: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/index.ts(9 hunks)src/types.ts(2 hunks)test/macro/macro.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
test/macro/macro.test.ts (1)
src/index.ts (3)
Elysia(186-8223)Elysia(8225-8225)options(6283-6372)
src/types.ts (1)
src/index.ts (2)
MacroIntrospectionMetadata(8318-8318)HTTPMethod(8297-8297)
src/index.ts (2)
src/types.ts (1)
MacroIntrospectionMetadata(1946-1961)src/utils.ts (2)
mergeDeep(52-103)isNotEmpty(1078-1084)
🔇 Additional comments (7)
test/macro/macro.test.ts (7)
1442-1531: LGTM! Comprehensive coverage of introspection styles.These tests effectively validate the three different ways to define macro introspection (shorthand styles 1 & 2, and longhand). The assertions correctly verify that introspection receives both the macro options and the route metadata (path and method).
1575-1603: LGTM! Validates introspect with resolve.This test confirms that introspection works correctly when macros also define
resolvehooks, which is an important edge case.
1806-1820: LGTM! Validates introspect is not called unnecessarily.This test correctly verifies that introspection is only invoked for routes that actually use the macro.
1822-1879: LGTM! Comprehensive validation of multiple macro introspection.This test effectively verifies that multiple macros can coexist and introspect independently, with each macro's introspect function called only for routes that use it.
1881-1903: LGTM! Validates introspect skips disabled macros.This test correctly verifies that introspection is not called when a macro value is
false.
1533-1573: Based on my verification, I can now provide the rewritten response:
The introspection behavior shown in this test is correct and intentional. The PR design includes two-phase introspection per route: guard-level macros are introspected once per route with full metadata, and route-level macros are also introspected once per route. When a route has conflicting macro options (like route-level
a: falseand guard-levela: true), both levels are appropriately introspected to capture complete metadata. The comments in the code (lines 286, 506) specifically refer to guard-level macros running "once per route"—not to a singular total introspection call per route. Multiple introspection calls are expected and correct when both guard and route-level macros apply to the same route.Likely an incorrect or invalid review comment.
1605-1804: Incorrect review comment: Macro path resolution behavior is intentional, not an inconsistency.The tests correctly reflect the designed behavior:
Groups defer introspection (via
skipMacroIntrospection = true, line 4033): Macros defined inside groups delay introspection until routes are added to the parent, enabling fully-resolved paths including the prefix.Guards do NOT defer introspection: The
guard()method (line 4527) does not setskipMacroIntrospection. Instead, it captures macro options from guard hooks for per-route introspection. Macros defined inside guard callbacks introspect immediately with local paths, which is why prefixes are excluded (the prefix belongs to the parent, not the guard scope).This design is documented in code comments:
- Group comment (line 4031-4032): "Macro introspection will happen when routes are added to the parent with the fully-resolved path"
- Guard comment (line 4537): "Capture guard-level macro options so we can introspect them per-route later"
The test expectations correctly verify this behavior:
- Tests 6-7: Group-scoped macros include prefix + group paths
- Tests 8-11: Guard-scoped macros exclude prefix (local scope only)
No changes needed.
Likely an incorrect or invalid review comment.
3f106f9 to
0771705
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/index.ts(11 hunks)src/types.ts(2 hunks)test/macro/macro.test.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/types.ts
🧰 Additional context used
🧬 Code graph analysis (2)
test/macro/macro.test.ts (1)
src/index.ts (4)
Elysia(186-8243)Elysia(8245-8245)options(6303-6392)group(4013-4152)
src/index.ts (2)
src/types.ts (1)
MacroIntrospectionMetadata(1946-1961)src/utils.ts (1)
isNotEmpty(1078-1084)
0771705 to
6543156
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/index.ts (1)
501-559:add()path normalization and guard-level introspection look correctNormalizing the path (leading
/and configured prefix) before introspection ensures macros see the same path the router uses, and gating guard macro introspection behind!this.skipMacroIntrospectionmatches the group/guard deferral design. Object-style macros withvalue === falseare correctly treated as disabled, and route-level macros still go throughapplyMacrowith{ path, method }metadata.If you ever want to avoid re-instantiating guard macro hooks per route, you could cache just the
introspectfunctions (and their static options) when capturingguardMacroOptions, but that’s an optional micro-optimization, not required for correctness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/index.ts(14 hunks)src/types.ts(2 hunks)test/macro/macro.test.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/types.ts
🧰 Additional context used
🧬 Code graph analysis (2)
test/macro/macro.test.ts (1)
src/index.ts (4)
Elysia(186-8315)Elysia(8317-8317)options(6375-6464)group(4024-4163)
src/index.ts (2)
src/types.ts (2)
MacroIntrospectionMetadata(1946-1961)AnyLocalHook(1680-1680)src/utils.ts (3)
mergeDeep(52-103)isNotEmpty(1078-1084)mergeHook(225-339)
🔇 Additional comments (8)
test/macro/macro.test.ts (2)
2-3: Imports switch tobun:testis appropriateUsing
describe/expect/itfrombun:testand importingElysiafrom../../srcis consistent with the rest of the test suite; no issues here.
1442-2025: New macro introspection tests give strong behavioral coverageThis block thoroughly exercises the new introspection flow: shorthand/longhand macros, guard-level macros (including conflicting/disabled cases), groups, nested guards/groups, routes without macros, multiple macros on a route, and stale
guard({ macro: true })state being cleared byguard({ macro: false }). The expectations match the implementation insrc/index.ts, so this looks solid.src/index.ts (6)
120-121:MacroIntrospectionMetadataimport/export is consistentImporting
MacroIntrospectionMetadatafrom./typesand re‑exporting it alongside other public types wires the new metadata surface cleanly into the public API without affecting existing types.Also applies to: 8410-8412
285-294: Internal guard/introspection state is scoped appropriatelyAdding
guardMacroOptionsandskipMacroIntrospectionasprotectedfields keeps the new state internal toElysiawhile enabling the per-route guard introspection plumbing you need. The comments clarify their purpose, and they don’t leak into generics or public APIs.
4045-4059: Group handling correctly defers introspection and propagates macrosMarking group child instances with
skipMacroIntrospection = trueand then merging bothextender.macroandinstance.guardMacroOptionsinto the parent before replaying routes ensures:
- macros defined inside groups are available when the parent introspects, and
- guard-level macros inside groups are introspected with fully resolved paths (prefix + group segments).
Restoring
guardMacroOptionsafterward avoids leaking group-specific guard state beyond the merge.Also applies to: 4075-4087, 4159-4161
4568-4598: 1-argguard(...): stale guard macro options bug is fixedThe new
updatedGuardMacroOptionslogic properly:
- deletes entries for object-style macros explicitly set to
false, and- only updates
this.guardMacroOptionswhen values change.This aligns with the tests that
guard({ a: false })should stop introspecting with stale{ a: true }, while still allowing function-style macros to usefalseas a legitimate option value.
4686-4720: 2-argguard(hook, run)preserves semantics and avoids double introspectionCopying
this.guardMacroOptionsinto the child instance, updating it based onhook, and then:
- running guard-level introspection on the child as routes are added, and
- temporarily setting
this.skipMacroIntrospection = trueand clearingthis.guardMacroOptionswhile merging routes back,ensures:
- per-route guard introspection happens once with the child’s view of the path,
- group-level deferral continues to work (since groups use
skipMacroIntrospectionon their child instances), and- macros are still applied to merged routes via
applyMacro(mergedHook, mergedHook)without re-running introspect.The new tests around conflicting guard macros and
guard({ macro: false })within nested guards/groups reflect this behavior well.Also applies to: 4744-4752, 4768-4798, 4808-4811
5567-5577:applyMacrointrospect handling cleanly separates guard vs route introspectionExtending
applyMacroto accept optionalmetadata: MacroIntrospectionMetadataand only invokingvalue?.(introspectOptions, metadata)when metadata is present:
- prevents guard-level macros from introspecting during guard registration (no metadata passed), and
- ensures route-level macros introspect with
{ path, method }when called fromadd().The recursive calls propagate the same metadata so nested macros also see correct route metadata. Deleting the macro key from
localHookregardless of whether metadata is present preserves previous macro application semantics.Also applies to: 5615-5625, 5643-5646
|
@SaltyAom I was able to update index.ts so that it now passes all of the new tests for this feature. However, I ended up making more changes than I would have preferred, including adding a few instance variables that I’m not fully convinced are necessary. If there’s a more elegant or idiomatic way to approach this within Elysia’s design, I’d be happy to revise the PR. |
Goal: Enable macros to introspect route metadata (path and method) during route registration for documentation generation and analysis.
Features Added:
Route metadata in macro introspection
MacroIntrospectionMetadatawithpathandmethodintrospectnow receives route metadata as a second parameterPath resolution
Guard-level macro introspection
Behavior Changes:
introspectreceives(introspectOptions, metadata)wheremetadatacontains{ path: string, method: HTTPMethod }/prefix/group/route/:id)Use Case: Enables macros to generate documentation, analyze routes, or perform static analysis using route path and method information.
Summary by CodeRabbit
New Features
Tests