-
Notifications
You must be signed in to change notification settings - Fork 180
feat(event-handler): implement route matching & resolution system for rest handler #4297
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
Conversation
1294c96 to
2d494eb
Compare
76bf247 to
9a1c059
Compare
7e78c3c to
c8d8bb4
Compare
dreamorosi
left a comment
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.
Amazing work here, especially with the deep dive in time complexity and optimization.
The code looks good even though I haven't tested it in a function, I think for now we can merge it and progress - either way we'll have a team bug bash when we're near the release.
The only thing potentially missing, which I am ok to tackle in a separate issue is type safety.
The linked issue had an ugly recursive type (which I could swear I saw a question from you about but can't find anymore) that basically aimed at giving the experience described in the RFC in the section "Dynamic routes" which would allow you to do this in the route handler:
As I said, I'm ok to work on this separately and in the near future - I have a local PoC with this working (in terms of types) that I made over a year ago when I was working on the RFC, so we can potentially take some inspiration there.
885dfb0 to
dd3d169
Compare
Yeah, I mentioned that recursive type in the PR body at the top, I wasn't sure what to do with it. That autocomplete use case is so nice, defo something we want! But agreed we can tackle it in a separate PR, this one is big enough as it is. |
dreamorosi
left a comment
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.
Thanks for working on this!
|



Summary
Implements the route matching and resolution system by following the pattern of the
appsync-eventhandler and centralising the resolution logic in theRouteHandlerRegistry. This different from the proposed solution in #4140, which envisioned multiple classes involved in the routing and matching logic. This design is simpler and has less moving parts.Changes
RouteHandlerRegistryregisterfunction is responsible for deciding which goes where.refreshmethod proposed that would have been called each time a route was added. This is also done in theregistermethod.resolvemethod takes a HTTP method and a pathnull.utilsmodule, although I am leaning towards moving them to be private methods in theRouteHandlerRegistryclass as I don't think they're going to be used anywhere else:processParams, which URL decodes any parameters in the pathvalidateParams, which throws a validation error if the parameters have invalid valuesNote
There was one part of the initial implementation that I wasn't able to incorporate. There was a recursive type that was defined but it was never used anywhere so I wasn't sure what to do with it:
Issue number: closes #4140
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.