-
Notifications
You must be signed in to change notification settings - Fork 4
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
Updates to handling of Requests #88
Conversation
…lization and improve request ob BREAKING CHANGE: Request class has changed
add functinality to support future features outlined in issue SpartanLabs#73 re SpartanLabs#86
src/request/request.ts
Outdated
import { HttpMethods } from './http-methods' | ||
|
||
export class Request<TBody = { [key: string]: any }> { | ||
protected _pathParam: Readonly<{ [key: string]: string }> |
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.
Think we should make all these maps?
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.
what is the value? It is a little bit of overhead (need to iterate through the whole object
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.
If we update our request event and request data to use maps it forces the users of this library to do the translation and I think I am fine with that
we would then be using es6 maps throughout which i like]
however, i think it is out of the scope of this PR and I will open an issue to address in the future
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.
on second thought it is completely within the scope of the PR lol
tsconfig.json
Outdated
@@ -11,6 +11,7 @@ | |||
"experimentalDecorators": true, | |||
"emitDecoratorMetadata": true, | |||
"declarationDir": "dist/types", | |||
"strictPropertyInitialization": false, |
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.
Why are we setting this to false?
src/routing/IRouter.ts
Outdated
> { | ||
route: TRoute | ||
metadata: RouteMetadata<TRoute> | ||
pathParameters: { [key: string]: string } |
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.
Time 4 map?
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.
could do that.. will need to somewhere translate them to maps, is it worth it? I suppose?
) {} | ||
|
||
resolve<T>(constructor: GenericConstructor<T>): T { | ||
return this.container.resolve(constructor) | ||
} | ||
|
||
async handleRequest(request: Request): Promise<Response> { | ||
async handleRequest(event: RequestEvent): Promise<Response> { | ||
const [data, error] = this.getRouteData(event) |
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.
why handle errors this way instead of wrapping this in a try catch block?
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.
dont remember 100% i think it was to basically make it so i didn't have to use let
and/or have a large try catch body,
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.
Is this really worth it?
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.
to avoid this?
let routeData: RouteData
try {
routeData = this.router.getRouteData(event)
} catch (e) {
this.errorHandler(error, event)
}
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.
good question
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.
Feel free to open a PR to change
error: Error, | ||
request: Request | RequestEvent | ||
): Response => { | ||
console.error({ request, error }) |
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.
Shouldn't logging this stuff be for the user to do
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.
yes this is just the default error handler in case one is not supplied. Users should supply their one error handler.
…o be es6 maps BREAKING CHANGE: must now pass headers and query string through as es6 maps
Pull Request Test Coverage Report for Build 211
💛 - Coveralls |
) {} | ||
|
||
resolve<T>(constructor: GenericConstructor<T>): T { | ||
return this.container.resolve(constructor) | ||
} | ||
|
||
async handleRequest(request: Request): Promise<Response> { | ||
async handleRequest(event: RequestEvent): Promise<Response> { | ||
const [data, error] = this.getRouteData(event) |
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.
Is this really worth it?
🎉 This PR is included in version 3.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Most of the changes are to support the features outlined in #73