Make ClientRequest chop off fragment identifiers from urls #972
Conversation
|
|
| console.error("An Invalid url was provided"); | ||
| } else { | ||
| this._url = match[1]; | ||
| } |
There was a problem hiding this comment.
Couple requests for this section:
- I think for the purposes of this PR, I'd prefer not to log anything in the case an "invalid" URL was provided. We could/should validate the URL that we're given, but if we were doing that I'd probably want to
throwrather than just log, and that's considerably more involved than stripping the hash off the end, so we can put that aside for now. - I'd like to keep the two assignments to
this._urlin constructor closer together. Feel free to move the first one down into your first if-case.
This doesn't apply to this specific PR, since I'd like to remove the new logging anyway, but for future react-server submissions, we've got a logger set up in react-server that ensures that we get log messages to the appropriate places w/ the appropriate severity levels. In general, rather than console.error, we should use that setup (if you search for "logger" in the code, it should be easy to find examples).
| clientRequest = new ClientRequest("/react-server/foo/?foo=bar&baz=123&zed=abc?#some-fragment?#"); | ||
| expect(clientRequest.getUrl()).toEqual("/react-server/foo/?foo=bar&baz=123&zed=abc?"); | ||
| done(); | ||
| }); |
There was a problem hiding this comment.
Thanks for adding tests!
One additional test I'd like to see here is a test that has a URL with a ? following the #. e.g. /react-server/foo#?bar=3&foo=7. I feel like I've seen libraries that store data in the hash in that format, and because we're looking for both # and ? in regexes in ClientRequest, it seems like a place we could have regressions in the future.
There was a problem hiding this comment.
In this case, what would the expected behavior be? Based on this:
https://en.wikipedia.org/wiki/Uniform_Resource_Identifier#Syntax
It seems like it will be wrong to store the part after the ? as as query params in this case since it does not comply with the uri syntax.
There was a problem hiding this comment.
Right, I'd expect that everything after the # is stripped -- it's still part of the hash, and not the query string. The concern would be that something in the code changes later, and all of a sudden we start matching the ? before the # or something, and it accidentally gets included as the query string. A test case should help prevent that kind of regression.
jleoirab
force-pushed
the
remove-hash-from-client-request-url
branch
from
b108f38 to
2388c6a
Compare
|
This looks good to me with the latest changes. Nice fix @j-beatzz. |
This PR tries to make
ClientRequestandExpressServerRequestconsistent by ensuring the that fragment identifiers are chopped of from urls inClientRequestand that they are not included whengetUrlis called.