feat(event-handler-core): give routes a name so one can be decorated - #5673
Merged
Merged
Conversation
`HttpRouteDefinition` gains a `name`. Decorators reach definitions — the router resolves them — so decorating `HttpRouteDefinition` hands a project every route in turn, and `name` is how it tells which one it has before swapping the handler or the path. This is the piece that made route decoration possible at all. Decorators do not reach `HttpRouteHandler`, because the router builds the matched handler class directly instead of resolving that shared abstraction, which is what keeps unmatched routes unbuilt. Until now there was no supported way to change one route. Framework routes get kebab-case names (`graphql`, `asset-delivery`, `background-task`, `cms-manage`). `<Api.Route>` reuses its existing `routeName` prop, so the DI name and the Pulumi resource name are the same value rather than two things that can drift. `deriveRouteName` moves into `routePath.ts` now that both consumers need it. Three tests cover the case that matters: decorating by name swaps only the named route, it can repoint a path, and — the one worth keeping — decorating definitions does NOT resurrect the eager-construction cost, since an unmatched route's handler is still never built. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
🚓 Slop Cop ✅ Nothing worth flagging. The diff looks consistent with the PR's stated intent and the code-style rules. The PR matches its stated intent — a broad but coherent, mechanical change (adding Automated, non-blocking heads-up from an LLM. It can be wrong — use your judgment. Regenerates on every push. |
…hy it exists The claim that a route's behaviour can be wrapped through its definition was only ever backed by a throwaway run. It is a test now: `HttpRouteDecoration.test.ts` asserts before/original/after ordering for a wrapper returned as a definition's `handler`. `buildHttpRoute`'s comment now states the actual mechanism rather than just the symptom. `resolveWithDependencies` is the one resolve path in `@webiny/di` that never calls `applyDecorators` — `resolveInternal`, `resolveRegistration` and `resolveMultiple` all do — which is exactly why `HttpRouteHandler` decorators reach nothing while `HttpRouteDefinition` decorators work. It also records what would remove the limitation: a DI method that resolves a specific implementation through the decorating path. The skill documents the wrapper, and `buildHttpRoute` / `RequestContainer` are exported from `webiny/api` so it can actually be written. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…he DI follow-up The skill's decorator used getters, which read as though `handler` were a method being overridden. It is a plain property on `IHttpRouteDefinition`, so the example now assigns the four properties in the constructor — plain fields work, verified — with a note that getters are equally fine. `buildHttpRoute`'s comment now says what the DI change would actually buy. If `resolveWithDependencies` applied decorators like every other resolve path, that would enable CROSS-CUTTING decoration: every route wrapped identically, for timing or logging. It would not replace decorating the definition, because a handler decorator receives only the route instance and the instance carries no name — so it cannot tell which route it is wrapping. Targeting one route stays a definition-level job either way. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
….route
`handle()` now receives `request.route` — `{ name, method, path }` of the
definition that matched, the `req.route` of Express-style handlers. The router
already builds the request it passes down, so this is a field on that object.
It makes targeting work at REQUEST time rather than wiring time. A wrapper can
be applied to every route and still act on one:
if (request.route.name !== "orders") {
return buildHttpRoute(this.container, inner).handle(request, response);
}
which reads better than picking the route out by name when registering the
decorator, and is the same body a plain `HttpRouteHandler` decorator would have
if DI applied decorators to `resolveWithDependencies`. It also removes the
objection to that change: a handler decorator cannot tell which route it wraps
from its constructor, but it does not need to, because the request says.
A route can also just read its own identity.
`route` is optional on `IHttpRequest`, since a transport builds a request before
anything has matched, and required on `HttpRouteHandler.Request`, which is what
handlers take — so inside `handle()` it needs no guard.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…n', not 'cross-cutting' A decorator applies to every implementation registered under its abstraction — verified — and all routes share HttpRouteHandler, so one decorator would wrap all of them. Saying that plainly is clearer than the jargon, and it pairs with the correction already in this comment: wrapping them all is not the same as being unable to target one, because request.route says which route is running. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…r decorator
You can decorate the handler after all — I was wrong three times over about this.
`buildHttpRoute` registers the matched route alone in a throwaway child scope and
resolves it there. `resolveInternal` checks the current container before walking
to its parent, so the only `HttpRouteHandler` in scope is that one route, and
everything else still comes from the parent: dependencies resolve normally and
decorators apply, because this is ordinary resolution rather than a side door
around it. Routes that did not match are still never built.
So the plain form works today, no DI change needed:
HttpRouteHandler.createDecorator({
decorator: class implements HttpRouteHandler.Interface {
constructor(private decoratee: HttpRouteHandler.Interface) {}
async handle(request, response) {
if (request.route.name !== "orders") {
return this.decoratee.handle(request, response);
}
// ...
}
},
dependencies: []
});
Three things fall out. `Metadata` and its cast are gone, which was the whole of
follow-up 2. The test asserting decorators could not reach routes is inverted —
it now asserts they do. And decorating the definition is no longer the way to
change behaviour; it stays the way to change what a route IS (swap the handler,
move the path), which the skill now separates.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The Request block still listed the pre-`route` shape. Adds the field, and notes the distinction that is easy to trip on: `route.path` is the PATTERN (`/orders/:orderId") while the top-level `path` is what was actually requested (`/orders/abc123"). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…hild scope `IMatchedRouteDefinition` says what it is — the definition the router matched, minus its handler — and does not read as a fourth route-ish concept next to `HttpRouteDefinition` and `HttpRouteHandler`. `buildHttpRoute`'s comment now leads with the problem instead of the mechanism: every route shares one abstraction, so no `resolve()` call can mean "this particular route", and `resolveAll()` builds all of them. A child container whose only `HttpRouteHandler` registration is this route makes `resolve()` unambiguous, while delegating everything else to the parent — which is why the route's dependencies still resolve normally and decorators still apply. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…mplementation
@webiny/di 1.1.0 adds `Container.resolveImplementation(impl)` — resolve one
implementation, with its own dependencies and with decorators applied. That is
exactly what the router needs, so `HttpRouter` now asks for the matched route
directly:
const route = this.container.resolveImplementation(definition.handler);
`buildHttpRoute` is deleted. It existed only to work around
`resolveWithDependencies` not applying decorators: it registered the route alone
in a throwaway child container so an ordinary `resolve()` would find just that
one. Both the helper and the child container are gone, along with the `Metadata`
plumbing that preceded them.
Two tests go with it — the ones showing how to wrap a route's behaviour by
returning a wrapper class from a definition decorator. That was the workaround
for handler decorators not firing; a plain `HttpRouteHandler` decorator does the
same job, and "wraps behaviour with a plain handler decorator, targeted by name"
already covers it. Keeping them would enshrine a workaround as a pattern.
`event-handler-core` peer-depends on `^1.1.0` rather than `^1.0.2`, since 1.0.2
satisfies the old range but lacks the method. It is the only package that calls
it; the rest move by lockfile.
Also adds `event-handler-core/src/exports/api.ts`. The `HttpRouteHandler` and
`HttpRouteDefinition` exports were hand-written into the GENERATED
`packages/webiny/src/api.ts`, which `generate-webiny-package` rightly wipes —
the package had no exports folder for the generator to read. With one,
`validate-webiny-package` passes and the package.json export map is correct,
which the hand-edit had missed entirely.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The bump left two resolutions in the lockfile: `^1.1.0` at 1.1.0 for the
workspaces, and `^1.0.2` still pinned at 1.0.2 for @webiny/stdlib, which then
got a nested copy. Two copies means two `Abstraction` classes, so passing one
across the boundary failed to typecheck:
Property '__type' is protected but type 'Abstraction<T>' is not a class
derived from 'Abstraction<T>'
`yarn up` only rewrites the workspaces' own descriptors, so stdlib's range kept
its old resolution even though 1.1.0 satisfies it. `yarn dedupe` collapses both
onto 1.1.0 and drops the nested copy.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
brunozoric
added a commit
that referenced
this pull request
Sep 10, 2026
The removal was correct against the tree it was made on: the barrel had no event-handler-core usage at that point, and adio reported the dependency as unused. The rebase brought in #5673, which restores those imports as deep paths (@webiny/event-handler-core/features/http/abstractions.js), so adio now reports the inverse - used in source but not listed. Restoring the dependency and regenerating the tsconfig references. This commit and the one it reverts cancel out and can both be dropped when the branch history is tidied. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Routes get a
name,request.routetells a handler which route is running, and route decoration works with a plain DI decorator.Decorating a route
Decorate
HttpRouteHandler. It applies to every route, andrequest.route.namepicks the one you mean:Drop the
namecheck and it wraps every route, which is what you want for timing or logging.Decorating
HttpRouteDefinitionis still there for a different job: changing what a route is rather than what it does — point it at a different handler, or move its path.nameandrequest.routeHttpRouteDefinitiongains aname. Framework routes get kebab-case ones (graphql,asset-delivery,cms-manage,background-task).<Api.Route>reuses its existingrouteNameprop rather than inventing a second naming concept, so a route's DI name and its Pulumi resource name are the same string —deriveRouteNamemoved into the sharedroutePath.ts, which is what guarantees they cannot drift.request.routecarries{ name, method, path }of the matched definition intohandle(), thereq.routeof Express-style handlers. Noteroute.pathis the PATTERN (/orders/:orderId) while the top-levelpathis what was requested (/orders/abc123). It is optional onIHttpRequest, because a transport builds a request before anything has matched, and required onHttpRouteHandler.Request, so handlers need no guard.How the matched route is built
HttpRouterresolves the matched route withcontainer.resolveImplementation(definition.handler), added in@webiny/di1.1.0 (webiny/di#17). It reads the route's dependencies from its own metadata and applies decorators, which is what makes the plain decorator above work.Getting there took three wrong turns, all visible in the commit history:
resolveWithDependencies+Metadata. It is the only resolve path in@webiny/dithat never callsapplyDecorators, so routes were silently undecoratable. I documented that as an inherent limitation. It was not.resolve()would find just that one. This worked and restored decoration, but allocated a container per matched route per request and leaned on child-before-parent lookup order rather than a stated contract.resolveImplementation. The actual fix, in the DI package where it belonged.buildHttpRouteis deleted along the way — it only ever existed to house those workarounds.Also
event-handler-corepeer-depends on@webiny/di@^1.1.0rather than^1.0.2, since 1.0.2 satisfies the old range but lacks the method. The other 60 packages move by lockfile.Two lockfile-level gotchas worth knowing:
yarn updoes not rewritepeerDependencies, and it leaves transitive ranges alone —@webiny/stdlib's^1.0.2kept resolving to 1.0.2 and got a nested copy, which meant twoAbstractionclasses and aProperty '__type' is protectedtype error.yarn dedupe @webiny/difixes it.event-handler-corealso gainssrc/exports/api.ts. TheHttpRouteHandler/HttpRouteDefinitionexports had been hand-written into the GENERATEDpackages/webiny/src/api.ts, whichgenerate-webiny-packagerightly wipes — the package had no exports folder for the generator to read. With one,validate-webiny-packagepasses and the package.json export map is correct, which the hand-edit had missed entirely.Verified
event-handler-core87,api-headless-cms920,api-core181,api-aco74,event-handler-aws61,api-graphql42,project-aws31, plus the smaller suites. Full uncached build clean.adio,format:check,oxlint,sync-dependencies,validate-webiny-packageall clean.The tests that matter most are the ones pinning behaviour that is easy to lose: an unmatched route is never built, a matched route's dependencies are, and decorators reach the matched route. Those survived all three implementations above unchanged, which is how each rewrite was checked.