-
-
Notifications
You must be signed in to change notification settings - Fork 87
Prevent removing trailing slash in request URL #96
Prevent removing trailing slash in request URL #96
Conversation
} | ||
|
||
function stripSlashes(path) { | ||
if (typeof path === 'string') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of checking for string 3 times, you could either type cast the argument to a string or throw an error if the argument it not a string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
throw an error if the argument it not a string
Which argument? The one to _buildURL
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I'm referring to typeof path === 'string'
. You don't need to check the argument type here.
It's safe enough for stripSlashes to either force path
argument to be a string or throw an error here if it's not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh okay, gotcha, I'll fix that up.
Is there a reason not to do the check? Like a performance concern? Or is it just unnecessary? removing the checks makes a bunch of tests fail that were passing before.
Other than one comment, it looks good. |
8319202
to
81aa92c
Compare
@taras can you review this again? I took care of the issue you mentioned. |
👍 |
The check was not actually whether or not a URL was absolute, but whether it was "complete". For example, "http://foo.com" should have been recognized, although that is not actually what an "absolute url" usually refers to, which is something like "/foo". The name change just increases the clarify of what is being verifyied.
The old way of building a URL was too brittle and didn't cover a lot of the different cases that could take place, in different combinations of the host, namespace, and segment having or not having slashes included. The new approach implemented here handles the various cases correctly, and ensures that we do not remove a trailing slash if one was provided. Closes ember-cli#70
81aa92c
to
b46efae
Compare
The old way of building a URL was too brittle and didn't cover a lot of the different cases that could take place, in different combinations of the host, namespace, and segment having or not having slashes included.
The new approach implemented here handles the various cases correctly, and ensures that we do not remove a trailing slash if one was provided.