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

useQueries constructs URLs with hash fragments and query strings out of order #93

Closed
timdorr opened this issue Oct 9, 2015 · 9 comments

Comments

@timdorr
Copy link
Member

timdorr commented Oct 9, 2015

Specifically, in appendQuery, it appends the query string after pathname, which could include a hash fragment. This causes issues Link in react-router to direct to the wrong URL.

@timdorr
Copy link
Member Author

timdorr commented Oct 9, 2015

Mentioned in remix-run/react-router#2224.

@mjackson
Copy link
Member

mjackson commented Oct 9, 2015

I wonder if useQueries should put a 3rd arg into createPath (and pushState, replaceState, etc.). So

history.createPath({ the: 'state' }, '/the/path', { the: 'query' }, '#the-hash')

That, to me, feels cleaner than

history.createPath({ the: 'state' }, '/the/path#the-hash', { the: 'query' })

@taion
Copy link
Contributor

taion commented Oct 9, 2015

Worth considering what this means w/r/t #16 - would we be okay with hash being a no-op arg in that context?

@mjackson
Copy link
Member

mjackson commented Oct 9, 2015

We just use the hash to build a URL. I don't see why they couldn't find some use for it on native if they wanted to.

@timdorr
Copy link
Member Author

timdorr commented Oct 9, 2015

@mjackson The only issue I have with adding another field is that it gets away from the function name. useQueries sounds like it's just for queries in your URL. Maybe if it were useLocations or if pushState accepted Location objects?

Maybe the short term fix is to not mangle pathnames with hash fragments and the longer term fix is a change to the API?

@mjackson
Copy link
Member

In my mind useQueries means "add query parsing/stringifying to my history object so I don't have to do it manually". I think the extra arg works in that case.

@timdorr
Copy link
Member Author

timdorr commented Oct 10, 2015

I wonder if this would fit with remix-run/react-router#2186. That is, if useQueries could enhance pushState to accept both string and object locations.

And I'm reminded that I miss keyword args from Ruby right now 😉

@mjackson
Copy link
Member

I actually had a similar thought, @timdorr. Everywhere we accept a path we should also accept an object. useQueries would add the ability to put a query property on that object.

@mjackson
Copy link
Member

I added the ability to use a { pathname, search, hash } object anywhere you can use a path.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 5, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants