Skip to content
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

Merged
merged 5 commits into from
Jul 29, 2019
Merged

Conversation

zRosenthal
Copy link
Member

  • Update Request object
  • Add ability to provide a body deserializer
  • Some cleanup and refactoring

Most of the changes are to support the features outlined in #73

…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
@zRosenthal zRosenthal requested review from shametim and moraneva April 28, 2019 21:06
import { HttpMethods } from './http-methods'

export class Request<TBody = { [key: string]: any }> {
protected _pathParam: Readonly<{ [key: string]: string }>
Copy link
Member

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?

Copy link
Member Author

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

Copy link
Member Author

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

Copy link
Member Author

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,
Copy link
Member

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?

> {
route: TRoute
metadata: RouteMetadata<TRoute>
pathParameters: { [key: string]: string }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Time 4 map?

Copy link
Member Author

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)
Copy link

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?

Copy link
Member Author

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,

Copy link
Member

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?

Copy link
Member Author

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)
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good question

Copy link
Member Author

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 })
Copy link

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

Copy link
Member Author

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
@coveralls
Copy link

coveralls commented Jul 26, 2019

Pull Request Test Coverage Report for Build 211

  • 95 of 97 (97.94%) changed or added relevant lines in 7 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-1.4%) to 98.174%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/error/default-error-handler.ts 1 2 50.0%
src/request/default-body-deserializer.ts 3 4 75.0%
Files with Coverage Reduction New Missed Lines %
src/error/default-error-handler.ts 1 33.33%
Totals Coverage Status
Change from base Build 206: -1.4%
Covered Lines: 161
Relevant Lines: 164

💛 - 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)
Copy link
Member

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?

@zRosenthal zRosenthal merged commit 1bc6af3 into SpartanLabs:master Jul 29, 2019
@zRosenthal
Copy link
Member Author

🎉 This PR is included in version 3.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants