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

.asUrlString() attaches "/" to url #126

Closed
nirsky opened this issue May 31, 2020 · 4 comments
Closed

.asUrlString() attaches "/" to url #126

nirsky opened this issue May 31, 2020 · 4 comments

Comments

@nirsky
Copy link

nirsky commented May 31, 2020

Hey,

As the title says.
Not sure if this by design or a bug, but I think it's very confusing.

example:

const MY_URL = env.get('BASE_URL').default('https://my.baseurl.com').asUrlString();
console.log(MY_URL); // https://my.baseurl.com/
@nirsky nirsky changed the title .asUrlString() attached "/" to url .asUrlString() attaches "/" to url May 31, 2020
@evanshortiss
Copy link
Owner

Internally asUrlString() uses the url module from Node.js core (which implements WHATWG URL), and that adds the trailing slash.

Here's an example from Node.js v14.3.0.

new URL('http://a.b.com').toString()
'http://a.b.com/'
new URL('http://a.b.com/hello').toString()
'http://a.b.com/hello'

I feel like it makes sense to add the trailing slash, since in reality you're making a request to /. Perhaps this should be noted in the README?

@nirsky
Copy link
Author

nirsky commented Jun 4, 2020

Probably worth mentioning. I spent a couple of minutes trying to understand why my request is failing.

@evanshortiss
Copy link
Owner

@nirsky gotcha, I think a PR in docs reminding developers to use the URL module/class when working with URLs is a good idea, e.g instead of:

var path = '/thing'
var baseUrl = env.get('MY_URL').asUrlString()

// Could create invalid URL with two slashes!
var fullUrl = baseUrl + path

doing this:

var path = '/thing'
var baseUrl = env.get('MY_URL').asUrlString()

// Will be correct, with no double forward slashes
var fullUrl = new URL(path, baseUrl).toString()

Would you like to PR it?

@evanshortiss evanshortiss mentioned this issue Jun 6, 2020
@evanshortiss
Copy link
Owner

@nirsky this will be addressed in docs in 6.2.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants