No encode, decode in createLocation

[pshrmn]

Removes the automatic encoding of pathnames, leaving that up to the browser.

Moves the decoding to createLocation because 1) that is now used by the getDOMLocation calls to create a location and 2) we should ensure that if a user pushes a location whose pathname is encoded, we decode it for consistency.

[mjackson]

Did you wanna leave this in?

[pshrmn]

Oh bother. That's the remnant of my debugging when I accidentally passed done to a describe call instead of an it call.

[pshrmn]

I'm not actually sure about this test case. It passes, but I'm not sure if the behavior is desirable. This only happens if the user passes a bad location object.

[mjackson]

Seems like the kind of thing we don't actually need to test. i.e. if anyone actually tried to do this I'd probably just tell them to use hash instead of putting it on the pathname.

[pshrmn]

I went ahead and added two encoding checks. My thoughts are:

Throwing when given a bad path but also creating locations from bad paths is inconsistent. We should probably just choose one.

I was curious how Express deals with bad percent-encoding and when I used a wildcard path, it failed, but with no path it worked. I think that users would typically provide a pattern to match against, so that would be in favor or throwing an error.

// when requesting /test%
app.use('*', function() {...}) // throws URIError
app.use(function() {...}) // did not throw

RFC 3986 2.4 states:

Because the percent ("%") character serves as the indicator for
percent-encoded octets, it must be percent-encoded as "%25" for that
octet to be used as data within a URI.

That, too, seems to indicate that we should treat a bad percent-encoding as an error.

[pshrmn]

If we try to create a location with a bad percent-encoding, we swallow the error, keep location.pathname encoded, and log a warning.

[pshrmn]

A slightly more useful error than the default Uncaught URIError: URI malformed

[pshrmn]

Updated to just throw in createLocation when a pathname cannot be decoded. I believe that this makes the most sense to do, although it can be easily changed if you think otherwise @mjackson.

Internally, createPath is always passed a location object that was created by createLocation, so just having createLocation throw should be good enough. Having said that, createPath is exported by history, so maybe it should throw on its own?

[pshrmn]

The obvious issue with history throwing when it encounters a malformed URI is that developers will need to try...catch their code if they want to recover from bad percent-encoding. This would make it difficult to display an error page that looks like it is in their website.

try {
  ReactDOM.render((
    <BrowserRouter>
      <App />
    </BrowserRouter
  ), holder)
} catch (e) {
  ReactDOM.render((
    <MalformedURIMessage />
  ), holder)
}

I still think that history should throw and we can point to the RFC 3986 path parser rules to justify that, but there will still probably be people who want to use malformed URIs for some reason. I don't necessarily like the idea of automatically using the encoded pathname if the decode fails, but I think that it might be useful in the future to add an explicit "no decode" mode.

[nil4]

I still think that history should throw and we can point to the RFC 3986 path parser rules to justify that, but there will still probably be people who want to use malformed URIs for some reason

@pshrmn I think this is the right approach; an app using an invalid/malformed path can always be fixed to encode correctly. Thank you for fixing this.

[pshrmn]

Just bumping this since it has been a while. Whether or not this is the best approach, something definitely should be done here since the current implementation (made by myself ☹️) breaks things.

[nil4]

@pshrmn I would love to see this added. 👍

[mjackson]

This looks good to me. Thanks, @pshrmn!

[mjackson]

Seems like this should probably be a patch release since we're just fixing a bug we introduced in 4.6.0. Would you agree?

[pshrmn]

I agree. More of a bug fix than "added functionality", so I don't think that it would warrant a minor bump.

[pshrmn]

As a minor addendum, the next release would also include #459, but that is also pretty much just a bug fix, so I don't think that it warrants a minor bump.

[pshrmn]

One more thing I just remembered is that #485 needs to be addressed before the next release.

[timdorr]

OK, just got that one in for you.

@mjackson Did you want to push out a 4.6.2 release or add me to npm so I can take care of that for you? (you probably should add Ryan, BTW 😄)

[mjackson]

Thanks, @timdorr. I'll go ahead and cut the release :)