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

Changed .field to use .send instead. #97

Closed
wants to merge 1 commit into from

Conversation

BenAychh
Copy link

instead of
.field('foo', 'bar')

It should be saying
.send({foo: 'bar'})

instead of
.field('foo', 'bar')

It should be saying
.send({foo: 'bar'})
@BenAychh
Copy link
Author

Issue #86

@meeber
Copy link
Contributor

meeber commented May 30, 2016

Thank you for the PR @BenAychh!

I just reviewed #86 and I think there might be something else going on here.

According to the SuperAgent documentation, .field() is for use with multipart requests.

I believe the POST variables are still being sent when .field() is used, they just aren't accessible in the res object in the same way as when they are sent via .send().

I just ran a quick test using a multipart-aware HTTP Post Dumping Service, and it detected the POST variable that I set via .field. I didn't pursue how to locate that in the res object, but I suspect it's possible. At the very least, I observed the multipart/form-data's Content-Length increasing as I added more fields.

it("test", function (done) {
  chai.request("http://posttestserver.com")
    .post("/post.php")
    .field("APPLES", "BANANAS")
    .end(function (err, res) {
      console.log(res);
      done();
    });
});

Therefore, I think the documentation should instead be updated to indicate that .field is for multipart requests.

Also, README.md is generated via a build script, so any documentation changes should actually take place in request.js.

@Doogiemuc @C00bra @keithamus Does this sound like a fair plan?

@Doogiemuc
Copy link

Sounds perfectly fine for me. Thank you for the deep and thorough analysis. I vote for merging your pull request. happy

@barraponto
Copy link
Contributor

I believe using .send defaults to sending a JSON encoded request body. If you want to form-encode, you have to explicitly .type('form'). Like this:

chai.request(app).post('/user/login').type('form').send({username, email})

@wzup
Copy link

wzup commented Jun 21, 2017

@barraponto
How do you know about .type('form')? There is nothing said about it in docs of this so called "plugin".

Because what you say actually does work:

.type('form')
.send({foo: 'bar'})

Thanks, @barraponto.
And no thanks to authors of the plugin.

@meeber
Copy link
Contributor

meeber commented Jun 21, 2017

@wzup I suspect he looked at the superagent documentation which is the underlying module powering chai-http. There are doubtlessly problems in this plugin's documentation; the creator hasn't been active in 4+ years. The documentation may have been correct that long ago. This plugin is 100% maintained by the community. If you wish to make it better and keep it current, please open a PR with corrections.

@barraponto
Copy link
Contributor

Should we update the PR to include type('form')?

@keithamus
Copy link
Member

Thanks for your work here @BenAychh and @barraponto 👏

@fedeolto
Copy link

@barraponto thank you so much for pointing out type('form) 🙏🏻

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.

None yet

7 participants