-
-
Notifications
You must be signed in to change notification settings - Fork 332
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
fixes #9: Can now change headers and body on client response #14
Conversation
@@ -29,6 +29,7 @@ function Client (opts) { | |||
this.bytes = 0 | |||
this.headers = {} | |||
this.startTime = [0, 0] | |||
this.body = this.opts.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.
I think you can avoid this.
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.
I'm not sure, as this will allow the user to assign to client.body
and when the rebuildRequest()
is called, it should pick up their assignments. However, I would prefer to take this out and just use .setBody(...)
so its is a consistent api for headers and 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.
I agree with consistency. We should probably rename rebuildRequest
to _rebuild()
.
Unit tests are missing. |
|
||
this._req = Object.keys(this.opts.headers) | ||
.map((key) => `${key}: ${this.opts.headers[key]}\r\n`) | ||
.reduce((acc, str) => acc + str, this._req) |
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.
This might be a perf issues, I know it's my code :(.
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.
Well, the request is built/cached once, unless users go crazy with the .setHeaders()
and .setBody()
call. If you this improved though, I'll need some guidance :)
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.
Let's talk about this later on :)
Can you also add an example (maybe in the examples folder) where you are using this API? |
You need to call
.setHeaders(newHeaders)
and.setBody(newBody)
for modifying these values. The reason you can't shouldn't just assign.body
is because they are used to build a cached request, and simply overwriting it would mean I would have to un-cache the request and build it every time the client makes the request, because wouldn't be able to effectively and efficiently detect when it has been changed... simply put, its a big no-no for perf. 😄If you do need to assign directly to the body though, it is possible, you just need to rebuild your request like so:
However, the preferred method of use is:
Also,
.headers
cant be reassigned to, as I'm not what the.headers
variable are for.