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

fixes #9: Can now change headers and body on client response #14

Merged
merged 6 commits into from
Jun 24, 2016

Conversation

GlenTiki
Copy link
Collaborator

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:

const instance = autocannon({
    url: url,
    connections: 1000,
    duration: 10
  }, finishedBench)

  instance.on('response', function (client, statusCode, returnBytes, responseTime) {
    client.setHeaders({ .. })
    client.body = ...
    client.rebuildRequest()
  })

However, the preferred method of use is:

const instance = autocannon({
    url: url,
    connections: 1000,
    duration: 10
  }, finishedBench)

  instance.on('response', function (client, statusCode, returnBytes, responseTime) {
    client.setHeaders(...)
    client.setBody(...)
  })

Also, .headers cant be reassigned to, as I'm not what the .headers variable are for.

@@ -29,6 +29,7 @@ function Client (opts) {
this.bytes = 0
this.headers = {}
this.startTime = [0, 0]
this.body = this.opts.body
Copy link
Owner

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.

Copy link
Collaborator Author

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.

Copy link
Owner

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().

@mcollina
Copy link
Owner

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

@mcollina mcollina Jun 23, 2016

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 :(.

Copy link
Collaborator Author

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

Copy link
Owner

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

@mcollina
Copy link
Owner

Can you also add an example (maybe in the examples folder) where you are using this API?

@mcollina mcollina merged commit dc63d0a into mcollina:master Jun 24, 2016
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.

2 participants