-
Notifications
You must be signed in to change notification settings - Fork 742
Add Feature expansion .use(path, middleware) #3044
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
Changes from 11 commits
e787f5b
9dc89c7
dded435
f6e30ce
4368e92
6225d6c
f470727
fb534fa
8a25677
0d038b2
472fdde
9288717
d606c3a
08332fe
c120113
fd94d90
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -172,8 +172,23 @@ export class App<State> { | |||||||||||||||||||
| return this; | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| use(middleware: MiddlewareFn<State>): this { | ||||||||||||||||||||
| this.#router.addMiddleware(middleware); | ||||||||||||||||||||
| use(pathOrMiddleware: MiddlewareFn<State>): this; | ||||||||||||||||||||
| use( | ||||||||||||||||||||
| pathOrMiddleware: string | URLPattern, | ||||||||||||||||||||
| middleware: MiddlewareFn<State>, | ||||||||||||||||||||
| ): this; | ||||||||||||||||||||
| use( | ||||||||||||||||||||
| pathOrMiddleware: string | URLPattern | MiddlewareFn<State>, | ||||||||||||||||||||
| middleware?: MiddlewareFn<State>, | ||||||||||||||||||||
| ): this { | ||||||||||||||||||||
| if (typeof pathOrMiddleware === "function") { | ||||||||||||||||||||
| this.#router.addMiddleware("/*", pathOrMiddleware); | ||||||||||||||||||||
| } else { | ||||||||||||||||||||
| if (!middleware) { | ||||||||||||||||||||
| throw new Error("Middleware is required when path is provided"); | ||||||||||||||||||||
| } | ||||||||||||||||||||
| this.#router.addMiddleware(pathOrMiddleware, middleware); | ||||||||||||||||||||
| } | ||||||||||||||||||||
| return this; | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
|
@@ -212,19 +227,44 @@ export class App<State> { | |||||||||||||||||||
| // - `app.mountApp("/*", otherApp)` | ||||||||||||||||||||
| const isSelf = path === "/*" || path === "/"; | ||||||||||||||||||||
| if (isSelf && middlewares.length > 0) { | ||||||||||||||||||||
| this.#router._middlewares.push(...middlewares); | ||||||||||||||||||||
| } | ||||||||||||||||||||
| // When mounting at the root, add middleware directly to the host app | ||||||||||||||||||||
| for (const middleware of middlewares) { | ||||||||||||||||||||
| this.#router._middlewares.push(middleware); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| for (let i = 0; i < routes.length; i++) { | ||||||||||||||||||||
| const route = routes[i]; | ||||||||||||||||||||
| // Add routes as-is since we're mounting at the root | ||||||||||||||||||||
| for (let i = 0; i < routes.length; i++) { | ||||||||||||||||||||
| const route = routes[i]; | ||||||||||||||||||||
marvinhagemeister marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||
| this.#router.add(route.method, route.path, [...route.handlers]); | ||||||||||||||||||||
| } | ||||||||||||||||||||
| } else { | ||||||||||||||||||||
| // For non-root mounts, we need to merge paths and apply middlewares to routes | ||||||||||||||||||||
| for (let i = 0; i < routes.length; i++) { | ||||||||||||||||||||
| const route = routes[i]; | ||||||||||||||||||||
| const merged = typeof route.path === "string" | ||||||||||||||||||||
| ? mergePaths(path, route.path) | ||||||||||||||||||||
| : mergePaths(path, route.path.pathname); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // If there are no middlewares, just add the route as-is with the merged path | ||||||||||||||||||||
| if (middlewares.length === 0) { | ||||||||||||||||||||
| this.#router.add(route.method, merged, [...route.handlers]); | ||||||||||||||||||||
| continue; | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // When there are middlewares, we need to add middleware handlers to each route | ||||||||||||||||||||
| // Extract middleware handlers into a flat array ensuring we get the right type | ||||||||||||||||||||
| const middlewareHandlers = middlewares.flatMap((middleware) => | ||||||||||||||||||||
| middleware.handlers | ||||||||||||||||||||
| ); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| const merged = typeof route.path === "string" | ||||||||||||||||||||
| ? mergePaths(path, route.path) | ||||||||||||||||||||
| : route.path; | ||||||||||||||||||||
| const combined = isSelf | ||||||||||||||||||||
| ? route.handlers | ||||||||||||||||||||
| : middlewares.concat(route.handlers); | ||||||||||||||||||||
| this.#router.add(route.method, merged, combined); | ||||||||||||||||||||
| // Create a new array of handlers with the correct type | ||||||||||||||||||||
| const combinedHandlers: Array<MiddlewareFn<State>> = [ | ||||||||||||||||||||
|
||||||||||||||||||||
| const combinedHandlers: Array<MiddlewareFn<State>> = [ | |
| const combinedHandlers = [ |
Nit: this might not be needed
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.
| // Create a new array of handlers with the correct type | |
| const combinedHandlers: Array<MiddlewareFn<State>> = [ | |
| ...middlewareHandlers, | |
| ...route.handlers.map((h) => h as unknown as MiddlewareFn<State>), | |
| ]; | |
| const combinedHandlers = [ | |
| ...middlewareHandlers, | |
| ...route.handlers, | |
| ]; |
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice tests!
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks! |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -16,8 +16,8 @@ export interface RouteResult<T> { | |||||
|
|
||||||
| export interface Router<T> { | ||||||
| _routes: Route<T>[]; | ||||||
| _middlewares: T[]; | ||||||
| addMiddleware(fn: T): void; | ||||||
| _middlewares: Route<T>[]; | ||||||
| addMiddleware(pathname: string | URLPattern, fn: T): void; | ||||||
| add( | ||||||
| method: Method | "ALL", | ||||||
| pathname: string | URLPattern, | ||||||
|
|
@@ -30,29 +30,37 @@ export const IS_PATTERN = /[*:{}+?()]/; | |||||
|
|
||||||
| export class UrlPatternRouter<T> implements Router<T> { | ||||||
| readonly _routes: Route<T>[] = []; | ||||||
| readonly _middlewares: T[] = []; | ||||||
| readonly _middlewares: Route<T>[] = []; | ||||||
|
|
||||||
| addMiddleware(fn: T): void { | ||||||
| this._middlewares.push(fn); | ||||||
| isURLPattern(value: unknown): value is URLPattern { | ||||||
| return value instanceof URLPattern; | ||||||
| } | ||||||
|
|
||||||
| add(method: Method | "ALL", pathname: string | URLPattern, handlers: T[]) { | ||||||
| if ( | ||||||
| typeof pathname === "string" && pathname !== "/*" && | ||||||
| IS_PATTERN.test(pathname) | ||||||
| ) { | ||||||
| this._routes.push({ | ||||||
| path: new URLPattern({ pathname }), | ||||||
| handlers, | ||||||
| method, | ||||||
| }); | ||||||
| } else { | ||||||
| this._routes.push({ | ||||||
| path: pathname, | ||||||
| handlers, | ||||||
| method, | ||||||
| }); | ||||||
| } | ||||||
| addMiddleware(pathname: string | URLPattern, handler: T): void { | ||||||
| this._middlewares.push({ | ||||||
| path: this.isURLPattern(pathname) | ||||||
| ? pathname | ||||||
| : new URLPattern({ pathname }), | ||||||
| handlers: [handler], | ||||||
| method: "ALL", | ||||||
| }); | ||||||
| } | ||||||
|
|
||||||
| add( | ||||||
| method: Method | "ALL", | ||||||
| pathname: string | URLPattern, | ||||||
| handlers: T[], | ||||||
| ): void { | ||||||
|
||||||
| ): void { | |
| ) { |
Nit: the Deno team consider defining void and Promise<void> as explicit return types as needless boilerplate.
Outdated
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.
Nit: To clarify, my idea was to have everything inside this condition be factored out. Not just the URLPattern bit. Also, I don't see a reason why it should be a method when it doesn't use any property from this class.
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.
Apologies — I didn’t fully grasp your intent at first.
I’ve now moved the logic out of the class and reorganized the content.
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.
Nit