Add support for options.qs #160

jamesplease · 2017-04-07T03:55:56Z

Just a quick proof-of-concept implementation of #159.

naugtur · 2017-04-07T07:22:33Z

README.md

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


Not a parser. I would call it a query string builder

Good catch.

naugtur · 2017-04-07T07:23:29Z

index.js

+    var qsStringifyDefined = isFunction(createXHR.queryStringStringify);
+
+    if (options.qs && !qsStringifyDefined) {
+      throw new Error("You passed a 'qs' option, but did not define an 'xhr.queryStringStringify' function.\nYou must either omit the 'qs' option, or define 'xhr.queryStringStringify'.")


I'll shorten this message if you don't mind.

No problem. Go for it.

naugtur · 2017-04-07T07:25:25Z

Looks good.

@reqshark @gsf Any comments? I'd merge it into v3 as is.

reqshark

my only nit is about term queryStringStringify. When we're talking about uri encoding it doesn't serve developers to use the same terms as JSON operations. Regardless of what popular libs do, uri encoding looks very different from JSON

reqshark · 2017-04-07T07:39:22Z

README.md

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


can we call this xhr.qs ?

reqshark · 2017-04-07T07:39:35Z

README.md

+a parser for `options.qs` out of the box: you must define it yourself as
+`xhr.queryStringStringify`.
+
+If `options.qs` is defined, and `xhr.queryStringStringify` is not, then an


xhr.qs please

reqshark · 2017-04-07T07:40:04Z

README.md

+assumptions about how to handle them, and does not support the `qs` option out
+of the box.
+
+To support the `qs` option, you must define an `xhr.queryStringStringify`


another xhr.qs

pls also remove you must, it reads better as:

"To support a qs option, define an xhr... function"

reqshark · 2017-04-07T07:40:06Z

README.md

+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.queryStringStringify`.


same comment

reqshark · 2017-04-07T07:40:52Z

README.md

+var xhr = require('xhr')
+var qs = require('qs')
+
+xhr.queryStringStringify = qs.stringify


xhr.qs = qs.stringify

reqshark · 2017-04-07T07:41:13Z

index.js

@@ -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.queryStringStringify = null // Define this as a function to support the `qs` option


createXHR.qs = null

reqshark · 2017-04-07T07:41:38Z

index.js

+      throw new Error("You passed a 'qs' option, but did not define an 'xhr.queryStringStringify' function.\nYou must either omit the 'qs' option, or define 'xhr.queryStringStringify'.")
+    }
+
+    var qs = options.qs && qsStringifyDefined ? '?' + createXHR.queryStringStringify(options.qs) : '';


same comment

reqshark · 2017-04-07T10:40:26Z

README.md

@@ -60,6 +60,7 @@ type XhrOptions = String | {
    withCredentials: Boolean?,
    responseType: String?,
    beforeSend: Function?
+    qs: Any?


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.

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

I'll leave it as-is for now. Y'all can hash this out in the v3 branch post-merge?

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

reqshark · 2017-04-07T10:40:42Z

README.md

+assumptions about how to handle them, and does not support the `qs` option out
+of the box.
+
+To support the `qs` option, you must define an `xhr.queryStringStringify`


pls also remove you must, it reads better as:

"To support a qs option, define an xhr... function"

jamesplease · 2017-04-07T10:53:22Z

@reqshark , i'm surprised you so strongly associate the word "stringify" with JSON. We don't have to use the term, but I think it is the best decision. I don't these other libs have made a mistake using it. Anything can be "stringified" – it's basically just a synonym for serializing, right?

Either way, I think using "qs" as both the option you pass when building an xhr, and as the builder/stringifier, could potentially cause confusion. Users have to deal with learning that "the xhr.qs function accepts options.qs and returns it as string." Not as straightforward as it could be imo.

How would you feel about any of these alternatives:

buildQs
qsBuild
serializeQs
qsSerialize

Another verb you used was "encode," but this function is not encoding anything imo, so i don't think we should consider that option.

Thanks for the review, you two! Feel free to push directly to this branch; otherwise, I'll update based on your comment tomorrow.

reqshark · 2017-04-07T11:00:18Z

Anything can be "stringified" – it's basically just a synonym for serializing, right?

I think using "qs" as both the option you pass when building an xhr, and as the builder/stringifier, could potentially cause confusion

ok fair points @jmeas, what about qsFn or just xhr.querystring?

reqshark · 2017-04-07T11:24:17Z

Another verb you used was "encode," but this function is not encoding anything imo, so i don't think we should consider that option.

the qs lib's stringify method encodes by default, and accepts encode options for encoding.

when forming XHR requests browser's encodeURIComponent() is helpful per RFC3986

the fact that you don't think "this function is encoding anything" gets precisely to my point about JSON terminology confusing people

naugtur · 2017-04-07T13:51:28Z

I'd support qsSerialize as my first choice.

naming things is hard. :)

jamesplease · 2017-04-07T16:58:19Z

I'll name it qsSerialize for now. Feel free to make changes in the v3 branch.

Would you like me to add tests in addition to these other changes you requested? Also, I made all of the suggested changes in the fixup PR (I think). Let me know if I missed anything.

I'll squash the commits when you give this PR your final 👍

naugtur · 2017-04-07T17:37:51Z

README.md

+var xhr = require('xhr')
+var qs = require('qs')
+
+xhr.qsSerialize = qs.stringify


^ 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

naugtur · 2017-04-07T17:38:35Z

index.js

+    var qsStringifyDefined = isFunction(createXHR.qsSerialize);
+
+    if (options.qs && !qsStringifyDefined) {
+      throw new Error("To use the 'qs' option, first define an 'xhr.qsSerialize' function.")


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

naugtur · 2017-04-07T17:40:35Z

I'm merging this manually to v3development.

jamesplease · 2017-04-07T19:10:51Z

Closing as this is merged into v3development: 807745f

gsf · 2017-04-08T06:40:17Z

Sorry I'm late to the party. To align with Node, qs, query-string, and request, the function reference should be qsStringify. That's the nomenclature. And aside from some creative edge cases, options.qs should be an Object, as it is in request.

Looking at the bigger picture, do we want to keep extending the API surface on the createXhr object? That moves us further away from a guiding factor in the simplicity of request: all configuration goes in that options object.

In a comment on #159, @jmeas makes a point that it's "a hard sell to use this lib for universal code, as the Node code would absolutely use the qs option." It's worth considering how committed we are to the "subset of request" goal. We could stay fully within the request API by pulling in qs as a dependency, but that obviously adds bloat that many may not need.

The qsStringify function could instead be set on options, similar to qsStringifyOptions in request. To avoid including it in every xhr call, we could add the rather cool request.defaults method. And if we add the undocumented requester argument to request.defaults, that solves the multiple signatures issue, making it just a few lines of code for anyone to add their own qs option:

var req = xhr.defaults(function (options, callback) {
  options.uri = (options.uri || options.url) + '?' + qs.stringify(options.qs);
  return xhr(options, callback);
});

naugtur · 2017-04-08T08:21:36Z

Valid points. Would you mind starting a new issue with this and tagging v3 milestone?
I think this also aligns with the goal of the most useful subset of request we can deliver on a tiny package.

jamesplease · 2017-04-08T17:25:29Z

@gsf, all of that sounds 👌 to me. Does the defaults option, as you've described it, require that all future requests be built from the req object?

gsf · 2017-04-08T21:24:49Z

Issue #163 created. And yes, in that example, the req function would be used for all future calls, just like a full wrapper.

Add support for options.qs

ebba3a9

naugtur reviewed Apr 7, 2017

View reviewed changes

reqshark reviewed Apr 7, 2017

View reviewed changes

fixup! Add support for options.qs

807745f

naugtur reviewed Apr 7, 2017

View reviewed changes

jamesplease closed this Apr 7, 2017

jamesplease mentioned this pull request Apr 7, 2017

Query string hook #159

Closed

jamesplease mentioned this pull request Aug 18, 2017

Support customization of query string serializer in redux-resource-xhr jamesplease/redux-resource#178

Closed

jamesplease mentioned this pull request Oct 20, 2017

Support customizing query string in redux-resource-xhr jamesplease/redux-resource#237

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for options.qs #160

Add support for options.qs #160

jamesplease commented Apr 7, 2017

naugtur Apr 7, 2017

jamesplease Apr 7, 2017

naugtur Apr 7, 2017

jamesplease Apr 7, 2017

naugtur commented Apr 7, 2017

reqshark left a comment

reqshark Apr 7, 2017

reqshark Apr 7, 2017

reqshark Apr 7, 2017

reqshark Apr 7, 2017

reqshark Apr 7, 2017

reqshark Apr 7, 2017

reqshark Apr 7, 2017

reqshark Apr 7, 2017

reqshark Apr 7, 2017

naugtur Apr 7, 2017

jamesplease Apr 7, 2017

naugtur Apr 7, 2017

reqshark Apr 7, 2017

reqshark Apr 7, 2017

jamesplease commented Apr 7, 2017 •

edited

Loading

reqshark commented Apr 7, 2017

reqshark commented Apr 7, 2017

naugtur commented Apr 7, 2017

jamesplease commented Apr 7, 2017

naugtur Apr 7, 2017

naugtur Apr 7, 2017

naugtur commented Apr 7, 2017

jamesplease commented Apr 7, 2017

gsf commented Apr 8, 2017

naugtur commented Apr 8, 2017

jamesplease commented Apr 8, 2017 •

edited

Loading

gsf commented Apr 8, 2017

Add support for options.qs #160

Add support for options.qs #160

Conversation

jamesplease commented Apr 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naugtur commented Apr 7, 2017

reqshark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesplease commented Apr 7, 2017 • edited Loading

reqshark commented Apr 7, 2017

reqshark commented Apr 7, 2017

naugtur commented Apr 7, 2017

jamesplease commented Apr 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naugtur commented Apr 7, 2017

jamesplease commented Apr 7, 2017

gsf commented Apr 8, 2017

naugtur commented Apr 8, 2017

jamesplease commented Apr 8, 2017 • edited Loading

gsf commented Apr 8, 2017

jamesplease commented Apr 7, 2017 •

edited

Loading

jamesplease commented Apr 8, 2017 •

edited

Loading