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.

Sign up for GitHub

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

Jump to bottom

Let the browser figure out the Content-Type when possible. #80

Merged
merged 1 commit into from
Jul 10, 2015
Merged

Let the browser figure out the Content-Type when possible. #80

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 10 additions & 4 deletions iron-ajax.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@
*/
contentType: {
type: String,
value: 'application/x-www-form-urlencoded'
value: null
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a (potentially undesired) side-effect of making this null by default: any observer that depends on this value will be called during initialization of the element. For example, _requestOptionsChanged will be called automatically when an iron-ajax instance is created, because null is assigned to this.contentType. See: https://github.com/PolymerElements/iron-ajax/blob/content-type/iron-ajax.html#L267-L270

In the case of iron-ajax, I don't think this will cause a bug immediately, but we have generally avoided default null unless it's really necessary.

Does the value have to be null by default, or will it suffice for it to default to undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense, but it looks like it prevents the _requestOptionsChanged observer from firing, because it's waiting for all arguments to be defined. As a result, the tests for auto fail.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, yes, that's true. I guess null is the best alternative, then.

},

/**
Expand All @@ -127,6 +127,7 @@
* Example:
*
* <iron-ajax method="POST" auto url="http://somesite.com"
* content-type="application/json"
* body='{"foo":1, "bar":2}'>
* </iron-ajax>
*/
Expand Down Expand Up @@ -317,9 +318,14 @@
* @return {Object}
*/
get requestHeaders() {
var headers = {
'Content-Type': this.contentType
};
var headers = {};
var contentType = this.contentType;
if (contentType == null && (typeof this.body === 'string')) {
contentType = 'application/x-www-form-urlencoded';
}
if (contentType) {
headers['Content-Type'] = contentType;
}
var header;

if (this.headers instanceof Object) {
Expand Down
74 changes: 74 additions & 0 deletions test/iron-ajax.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,17 @@
debounce-duration="150"></iron-ajax>
</template>
</test-fixture>
<!-- note(rictic):
This makes us dependent on a third-party server, but we need to be able
to check what headers the browser actually sends on the wire.
If necessary we can spin up our own httpbin server, as the code is open
source.
-->
Copy link
Contributor

Choose a reason for hiding this comment

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

The tests below are certainly important, but the dependency on a third party server in this case is very unhygienic. We don't really have a good strategy in place to tell WCT to spin up additional processes before running tests, either. What do you think the long term plan for solving this problem should look like?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need only the most minimal of servers for these tests. ~20 lines of node should do it if we wanted to run the server ourselves as part of WCT. I don't think it would be unreasonable for WCT to have a little local httpbin, it could be useful beyond just iron-ajax.

There are even a few projects where folks have started on implementing httpbin in node.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like it. Hopefully we can do that sooner rather than later.

<test-fixture id="RealPost">
<template>
<iron-ajax method="POST" url="http://httpbin.org/post"></iron-ajax>
</template>
</test-fixture>
<script>
suite('<iron-ajax>', function () {
var responseHeaders = {
Expand Down Expand Up @@ -313,6 +324,7 @@
suite('when making POST requests', function() {
setup(function() {
ajax = fixture('TrivialPost');
realAjax = fixture('RealPost');
});

test('POSTs the value of the `body` attribute', function() {
Expand Down Expand Up @@ -460,6 +472,68 @@

});

suite('when making a POST over the wire', function() {
test('FormData is handled correctly', function() {
server.restore();
var requestBody = new FormData();
requestBody.append('a', 'foo');
requestBody.append('b', 'bar');

var ajax = fixture('RealPost');
ajax.body = requestBody;
return ajax.generateRequest().completes.then(function() {
expect(ajax.lastResponse.headers['Content-Type']).to.match(
/^multipart\/form-data; boundary=.*$/);

expect(ajax.lastResponse.form.a).to.be.equal('foo');
expect(ajax.lastResponse.form.b).to.be.equal('bar');
});
});

test('json is handled correctly', function() {
server.restore();
var ajax = fixture('RealPost');
ajax.body = JSON.stringify({a: 'foo', b: 'bar'});
ajax.contentType = 'application/json';
return ajax.generateRequest().completes.then(function() {
expect(ajax.lastResponse.headers['Content-Type']).to.match(
/^application\/json(;.*)?$/);
expect(ajax.lastResponse.json.a).to.be.equal('foo');
expect(ajax.lastResponse.json.b).to.be.equal('bar');
});
});

test('urlencoded data is handled correctly', function() {
server.restore();
var ajax = fixture('RealPost');
ajax.body = 'a=foo&b=bar';
return ajax.generateRequest().completes.then(function() {
expect(ajax.lastResponse.headers['Content-Type']).to.match(
/^application\/x-www-form-urlencoded(;.*)?$/);

expect(ajax.lastResponse.form.a).to.be.equal('foo');
expect(ajax.lastResponse.form.b).to.be.equal('bar');
});
});

test('xml is handled correctly', function() {
server.restore();
var ajax = fixture('RealPost');

var xmlDoc = document.implementation.createDocument(
null, "foo", null);
var node = xmlDoc.createElement("bar");
node.setAttribute("name" , "baz");
xmlDoc.documentElement.appendChild(node);
ajax.body = xmlDoc;
return ajax.generateRequest().completes.then(function() {
expect(ajax.lastResponse.headers['Content-Type']).to.match(
/^application\/xml(;.*)?$/);
expect(ajax.lastResponse.data).to.match(
/<foo\s*><bar\s+name="baz"\s*\/><\/foo\s*>/);
});
});
});
});
</script>

Expand Down