Skip to content

Conversation

@mwhite
Copy link

@mwhite mwhite commented Jan 11, 2016

I changed the Sinon dependency URL because npm had a problem installing the git one, not sure if that's an issue with my configuration or not.

Copy link
Owner

Choose a reason for hiding this comment

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

I don't think we should be polyfilling this. We can add a note to the docs

Copy link
Author

Choose a reason for hiding this comment

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

Converted to a local function. Do you still prefer adding note to docs over an extra couple lines of code?

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, please remove the function entirely. IE9 has been EOL'd, and it's trivial to add in a polyfill. Most code these days is targeting ES5+ (including this project, with it's reliance on bind), no need for the extra bloat.

@akre54
Copy link
Owner

akre54 commented Jan 11, 2016

This would be fine with me, but I'm wondering if we should expose the stringify method to allow it to be overridden by the developer.

There are many ways to skin this particular cat (a=1&b=2, a=[1,2], a=1,2, to name a few), and it might be nice to give the dev some choice.

@mwhite
Copy link
Author

mwhite commented Jan 11, 2016

Are a=[1,2] and a=1,2 actually commonly used? It seems like for those three formats, it would be appropriate for the developer to handle conversion themselves, whereas this provides sensible behavior if you do want multiple parameter values with the same key. There's also jQuery's default, a[]=1&a[]=2, which can be switched to a=1&a=2 using $.ajaxSetup({traditional: true}).

mwhite and others added 6 commits January 28, 2016 17:48
if (typeof define === 'function' && define.amd) {
define(factory.bind(this, this.XMLHttpRequest));
} else if (typeof exports === 'object') {
module.exports = factory(require('xmlhttprequest').XMLHttpRequest);
Copy link
Owner

Choose a reason for hiding this comment

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

This shouldn't be necessary now. If it's only for testing, let's do that in the tests file.

@akre54
Copy link
Owner

akre54 commented Mar 14, 2016

I almost would rather have the dev write their own stringify logic if doing something custom. It's not super hard, should be as simple as:

import ajax from 'backbone.ajax';
import { stringify } from querystring;

Backbone.ajax = function(options) {
  return ajax(_.extend(options, {url: options.url + stringify(options.data)}));
};

@mwhite
Copy link
Author

mwhite commented Mar 14, 2016

Whoops, sorry, the later xmlhttprequest changes were not intended to be PR'd/automatically added to this PR. I'll close and resubmit if you're still interested.

@akre54
Copy link
Owner

akre54 commented Mar 15, 2016

No need to close, just amend your pull request by rebasing against master and force-pushing to this branch.

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