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

Query string arguments not supported in makePath #20

Closed
carystanley opened this issue Apr 23, 2015 · 7 comments
Closed

Query string arguments not supported in makePath #20

carystanley opened this issue Apr 23, 2015 · 7 comments
Assignees

Comments

@carystanley
Copy link

The use of "reverend" in MakePath does not support having querystring params.

@lingyan
Copy link
Member

lingyan commented Apr 24, 2015

Yup. reverend only generates path, not the query string. But we can use libraries like qs to construct the query string param and tag it onto the path that's generated.

@celineotter
Copy link

@lingyan - please could you provide a brief example of how these two connect together ?

@lingyan
Copy link
Member

lingyan commented Aug 7, 2015

Hi @celineotter,

routr no longer uese reverend. It is now using path-to-regexp for makePath: https://github.com/yahoo/routr/blob/master/lib/router.js#L141-L165

makePath is the function used by fluxible-router to create the route's path in NavLink: https://github.com/yahoo/fluxible-router/blob/af15b8c3e2b9149824659c9cdb444c8b591e70de/lib/createNavLinkComponent.js#L91

So that application can provide routeName and navParams to NavLink, which will generate the href for the anchor using these 2 props.

This issue is a feature request for extending the makePath to be able to take also query params, and generate the href that contains both path and query string.

I hope this is what you meant by "how these two connect together" :)

@lingyan
Copy link
Member

lingyan commented Aug 7, 2015

BTW, we can consider adding the support. The tricky thing is depending on which query string library you use, the query string generated is actually different. For example, between qs (used by express and hapi) and node's built-in querystring:

// qs_querystring.js
var qs = require('qs');
var querystring = require('querystring');

var k = {
    a: 1,
    b: false,
    c: {cc: 1},
    d: [1,2,3]
};
console.log('qs result is:', qs.stringify(k));
console.log('querystring result is:', querystring.stringify(k));

Running this simple script will output:

$ node qs_querystring.js 
qs result is: a=1&b=false&c%5Bcc%5D=1&d%5B0%5D=1&d%5B1%5D=2&d%5B2%5D=3
querystring result is: a=1&b=false&c=&d=1&d=2&d=3

I'd prefer using qs, as both express and hapijs use it.

Another option to get around this would be asking developer to pass a pre-built query string, not query params object.

@bchelli
Copy link

bchelli commented Aug 13, 2015

Hi Y'all,

👍 for qs

This is a feature I really miss in routr and more especially in fluxible-router.
YahooArchive/fluxible-router#74

Is anyone working on a pull request?

Thanks,
Ben

@redonkulus
Copy link
Contributor

@bchelli no one is at the moment, if you would like to try it out and open a PR, that would be great!

@AaronLasseigne
Copy link

Instead of picking a library we could ask users to provide it in the constructor. It must provide a stringify function. If it is not provided and a user attempts to provide query parameters an error is thrown. If this sounds good I'm willing to work on a PR to implement it. Then of course a PR in fluxible-router to use the functionality there.

@mridgway mridgway self-assigned this Mar 16, 2016
mridgway added a commit that referenced this issue Mar 16, 2016
[resolves #20] Add support for parsing and constructing urls with query params
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants