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

Decode and encode URLs #442

Merged
merged 2 commits into from
Mar 8, 2017
Merged

Decode and encode URLs #442

merged 2 commits into from
Mar 8, 2017

Conversation

pshrmn
Copy link
Contributor

@pshrmn pshrmn commented Mar 7, 2017

When a history gets the URL from the browser, the location object created from it should have decoded properties. When a history creates a URL (by calling its createHref method), the URL returned should be encoded. The actual changes to the code are fairly simple. parsePath calls decodeURI and createPath calls encodeURI.

When calling push and the path is a string, parsePath is called by createLocation, so that is how I tested that. createPath is tested via createHref.

In case you were curious, the translations (all in Japanese via Google Translate) are:
"歴史" is "history"
"キー" is "key"
"値" is "value"
"ハッシュ" is "hash"

@pshrmn
Copy link
Contributor Author

pshrmn commented Mar 7, 2017

One possible issue is that a location object that contains encoded values will be double-encoded when creating a URL. I feel like location objects should only contain decoded values, so I don't think that this is a big issue, but I don't know.

@timdorr
Copy link
Member

timdorr commented Mar 7, 2017

Makes perfect sense to me, given a URI is the unstructured, serialized string form of a structured location object. We should be encoding when creating a path/href. And it follows that we should also be decoding when parsing that path/href.

I'll take a look through the code in more detail now, but this is logically sound.

@pshrmn
Copy link
Contributor Author

pshrmn commented Mar 7, 2017

I'm not entirely confident in createPath. qs encodes strings automatically. It might be safer for createPath to only encode the pathname.

@mjackson
Copy link
Member

mjackson commented Mar 8, 2017

Seems like we just tell people using qs to qs.stringify(query, { encode: false }) in the router...

@mjackson mjackson merged commit babf4b4 into remix-run:master Mar 8, 2017
@mjackson
Copy link
Member

mjackson commented Mar 8, 2017

Thanks for the PR, @pshrmn :)

@pshrmn pshrmn deleted the decode branch March 8, 2017 03:44
@pshrmn
Copy link
Contributor Author

pshrmn commented Mar 8, 2017

I think that part of why I'm wary about encoding the search string is that React Router otherwise does not care about the search string, so forcing it to be decoded feels out of place.

Also, besides qs, query-string encodes by default and I didn't even see a way to not encode with querystring.

Another potential issue is if someone were to have a character that is not encoded by encodeURI (e.g. ; , / ? : @ & = + $ #), then the URL that creates could potentially be breaking.

{ pathname: '/call', search='?number=#867-5309', hash: '' }
// creates the URL /call?number=#867-5309
{ pathname: '/call', search='?number=', hash: '#867-5309' }

If we expected the user to correctly encode their own search strings, then we would not have to worry about that sort of problem.

Additionally, I'm not actually sure we need to be encoding any part of the path. Browsers seem to handle that for us perfectly fine.

@LukeAskew
Copy link

I think createPath should not be fully encoded - that removes control from upstream.

Given a setup where the query string looks something like ?filters[0]=foo&filters[1]=bar, when passing that value to .push() its encoded and the url displayed in the browser is filters%5B0%5D=foo&filters%5B1%5D=bar.

I have no way to control the format of the search string once its passed into .push().

@mjackson
Copy link
Member

@LukeAskew Why do you need to control the format of the search string?

@LukeAskew
Copy link

LukeAskew commented Mar 15, 2017

I have some utils for formatting the search string, similar to qs.

Here's a distilled version of the setup:

const history = createBrowserHistory();

// returns `filters[0]=foo&filters[1]=bar`
function stringify(obj) {
  const str = doSomeStuff(obj);
  return decodeURIComponent(str);
}

function updateQueryString(obj) {
  return history.push({ pathname: window.location.pathname, search: `?${stringify(obj)}` });
}

updateQueryString({
  filters: ['foo', 'bar'],
});

I don't want push to encode the brackets - I want the brackets in my search string.

@pshrmn
Copy link
Contributor Author

pshrmn commented Mar 15, 2017

FWIW I also think that we should only be encoding the pathname (#445).

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

Successfully merging this pull request may close these issues.

None yet

4 participants