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

Add support for options.qs #160

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ type XhrOptions = String | {
withCredentials: Boolean?,
responseType: String?,
beforeSend: Function?
qs: Any?
Copy link
Contributor

Choose a reason for hiding this comment

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

keep consistent w/ readme and avoid confusion of a non-type, i.e. qs: Function? here

To support the qs option, you must define an xhr.queryStringStringify function. This function accepts the value of options.qs as its first argument, and returns a string that is appended to the URL.

Copy link
Owner

Choose a reason for hiding this comment

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

I disagree here, the type of qs is unknown until you specify the xhr.queryStringStringify or xhr.qs function.
You can specify a function accepting an array or an object or a specific structure or a Map...

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'll leave it as-is for now. Y'all can hash this out in the v3 branch post-merge?

Copy link
Owner

Choose a reason for hiding this comment

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

sure

Copy link
Contributor

Choose a reason for hiding this comment

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

@naugtur @jmeas yea you guys are right about that, have to agree, leave it as Any?

}
xhr := (XhrOptions, Callback<Response>) => Request
```
Expand Down Expand Up @@ -193,6 +194,17 @@ A function being called right before the `send` method of the `XMLHttpRequest` o

Pass an `XMLHttpRequest` object (or something that acts like one) to use instead of constructing a new one using the `XMLHttpRequest` or `XDomainRequest` constructors. Useful for testing.

### `options.qs`

A value to be transformed into a query string. This library does not provide
a serializer for `options.qs` out of the box: you must define it yourself as
`xhr.qsSerialize`.

If `options.qs` is defined, and `xhr.qsSerialize` is not, then an
Error will be thrown.

For more, see [Query string support](#query-string-support).

## FAQ

- Why is my server's JSON response not parsed? I returned the right content-type.
Expand All @@ -216,6 +228,8 @@ xhr({
}
})
```
- How can I support query strings?
- See [Query string support](#query-string-support)

## Mocking Requests
You can override the constructor used to create new requests for testing. When you're making a new request:
Expand All @@ -231,6 +245,31 @@ xhr.XMLHttpRequest = MockXMLHttpRequest
xhr.XDomainRequest = MockXDomainRequest
```

## Query string support
There are many ways to stringify query parameters; consequently, `xhr` makes no
assumptions about how to handle them, and does not support the `qs` option out
of the box.

To support the `qs` option, define an `xhr.qsSerialize` function. This function
accepts the value of `options.qs` as its first argument, and returns a string
that is appended to the URL.

You do not need to include a leading "?" in the value that you return from
`xhr.qsSerialize`.

```js
var xhr = require('xhr')
var qs = require('qs')

xhr.qsSerialize = qs.stringify
Copy link
Owner

Choose a reason for hiding this comment

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

^ this line is very convincing towards naming it qsStringify after all.
I'll keep thinking about it after I merge this PR into v3 branch


xhr.get('/foo', {
qs: {
bar: true
}
})
```

## MIT Licenced

[1]: http://xhr.spec.whatwg.org/#the-send()-method
Expand Down
11 changes: 10 additions & 1 deletion index.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ var xtend = require("xtend")
module.exports = createXHR
createXHR.XMLHttpRequest = window.XMLHttpRequest || noop
createXHR.XDomainRequest = "withCredentials" in (new createXHR.XMLHttpRequest()) ? createXHR.XMLHttpRequest : window.XDomainRequest
createXHR.qsSerialize = null // Define this as a function to support the `qs` option

forEachArray(["get", "put", "post", "patch", "head", "delete"], function(method) {
createXHR[method === "delete" ? "del" : method] = function(uri, options, callback) {
Expand Down Expand Up @@ -139,9 +140,17 @@ function _createXHR(options) {
}
}

var qsStringifyDefined = isFunction(createXHR.qsSerialize);

if (options.qs && !qsStringifyDefined) {
throw new Error("To use the 'qs' option, first define an 'xhr.qsSerialize' function.")
Copy link
Owner

Choose a reason for hiding this comment

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

Great, I don't have to redact it anymore - exactly what I wanted to do.

}

var qs = options.qs && qsStringifyDefined ? '?' + createXHR.qsSerialize(options.qs) : ''

var key
var aborted
var uri = xhr.url = options.uri || options.url
var uri = xhr.url = (options.uri || options.url) + qs
var method = xhr.method = options.method || "GET"
var body = options.body || options.data
var headers = xhr.headers = options.headers || {}
Expand Down