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

URL cannot handle "localhost" domain for base url #26019

Closed
phallguy opened this issue Aug 11, 2019 · 10 comments
Closed

URL cannot handle "localhost" domain for base url #26019

phallguy opened this issue Aug 11, 2019 · 10 comments
Labels
Bug Stale There has been a lack of activity on this issue and it may be closed soon.

Comments

@phallguy
Copy link

URL constructor raises a TypeError when using localhost as the base url.

React Native version:

System:
OS: macOS 10.14.6
CPU: (12) x64 Intel(R) Core(TM) i9-8950HK CPU @ 2.90GHz
Memory: 4.55 GB / 16.00 GB
Shell: 3.2.57 - /bin/bash
Binaries:
Node: 12.6.0 - /usr/local/bin/node
Yarn: 1.17.3 - /usr/local/bin/yarn
npm: 6.9.0 - /usr/local/bin/npm
Watchman: 4.9.0 - /usr/local/bin/watchman
SDKs:
iOS SDK:
Platforms: iOS 12.4, macOS 10.14, tvOS 12.4, watchOS 5.3
IDEs:
Android Studio: 3.4 AI-183.6156.11.34.5692245
Xcode: 10.3/10G8 - /usr/bin/xcodebuild
npmPackages:
react: ^16.9.0 => 16.9.0
react-native: 0.60.4 => 0.60.4
npmGlobalPackages:
react-native-cli: 2.0.1

Steps To Reproduce

new URL("/home", "http://localhost:3000").toString() // TypeError invalid base URL

Describe what you expected to happen:

new URL("/home", "http://localhost:3000").toString() // "http://localhost:3000/home"

Snack, code example, screenshot, or link to a repository:

https://snack.expo.io/@phallguy/e6bc36

@jeswinsimon
Copy link
Contributor

The current regex in validateBaseUrl expects a base url to be of the format domain-name.extension.

I am not sure if the right solution is to accept all domains without domain extension or simply grant an exception for localhost. If it is the later then updating the regex to below will work.

/^(?:(?:(?:https?|ftp):)?\/\/)(?:(?:[1-9]\d?|1\d\d|2[01]\d|22[0-3])(?:\.(?:1?\d{1,2}|2[0-4]\d|25[0-5])){2}(?:\.(?:[1-9]\d?|1\d\d|2[0-4]\d|25[0-4]))|(?:(?:[a-z\u00a1-\uffff0-9]-*)*[a-z\u00a1-\uffff0-9]+)(?:\.(?:[a-z\u00a1-\uffff0-9]-*)*[a-z\u00a1-\uffff0-9]+)*(?:\.(?:[a-z\u00a1-\uffff]{2,}))|(?:localhost))(?::\d{2,5})?(?:[/?#]\S*)?$/i

@phallguy
Copy link
Author

Bare hosts are certainly valid URLs and accepted by URL in the browser.

new URL("/home", "http://phallguy").toString() // => http://phallguy/home

I don't really know why there's a custom-built URL for react-native so don't know if there are other issues to consider. Personally, I would expect feature parity with browser URL.

@jeswinsimon
Copy link
Contributor

Bare hosts are certainly valid URLs and accepted by URL in the browser.

If that is the case then I think the below will be a better solution

^(?:(?:(?:https?|ftp):)?\/\/)(?:(?:[1-9]\d?|1\d\d|2[01]\d|22[0-3])(?:\.(?:1?\d{1,2}|2[0-4]\d|25[0-5])){2}(?:\.(?:[1-9]\d?|1\d\d|2[0-4]\d|25[0-4]))|(?:(?:[a-z\u00a1-\uffff0-9]-*)*[a-z\u00a1-\uffff0-9]+)(?:\.(?:[a-z\u00a1-\uffff0-9]-*)*[a-z\u00a1-\uffff0-9]+)*(?:\.(?:[a-z\u00a1-\uffff]{2,}))?)(?::\d{2,5})?(?:[/?#]\S*)?$

@AntonioErdeljac
Copy link

AntonioErdeljac commented Aug 12, 2019

Bare hosts are accepted in WHATWG URL API as well, so I think it would make sense to allow it in React Native's URL constructor. https://nodejs.org/api/url.html#url_the_whatwg_url_api

@charpeni
Copy link
Contributor

Y'all probably want to check this #25719.

@jeswinsimon
Copy link
Contributor

@charpeni Thanks! This clears up a lot of my queries about why and how URL is implemented in React Native. I will close the PR #26050 once #25719 is resolved. Leaving it open just in case.

facebook-github-bot pushed a commit that referenced this issue Sep 11, 2019
Summary:
Fix for #26019 URL cannot handle "localhost" domain for base url. Also noticed another issue where `/` is not added between base URL and path if it is missing. Added fix for that too.

## Changelog

[Javascript] [Fixed] - `URL`: Bare Hosts are now supported.
Pull Request resolved: #26050

Test Plan:
* `new URL('home', 'http://localhost')` now returns `http://localhost/home` instead of throwing an error.
* `new URL('en-US/docs', 'https://developer.mozilla.org')` now returns `https://developer.mozilla.org/en-US/docs` and not `https://developer.mozilla.orgen-US/docs`.

Differential Revision: D17314137

Pulled By: cpojer

fbshipit-source-id: ef56c4f4032187a7efee32b28e2b3c935b6a2599
@stale
Copy link

stale bot commented Nov 12, 2019

Hey there, it looks like there has been no activity on this issue recently. Has the issue been fixed, or does it still require the community's attention? This issue may be closed if no further activity occurs. You may also label this issue as a "Discussion" or add it to the "Backlog" and I will leave it open. Thank you for your contributions.

@stale stale bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label Nov 12, 2019
@charpeni charpeni removed the Stale There has been a lack of activity on this issue and it may be closed soon. label Nov 12, 2019
@charpeni
Copy link
Contributor

👋 You should check this out: https://github.com/charpeni/react-native-url-polyfill.

@stale
Copy link

stale bot commented Feb 23, 2020

Hey there, it looks like there has been no activity on this issue recently. Has the issue been fixed, or does it still require the community's attention? This issue may be closed if no further activity occurs. You may also label this issue as a "Discussion" or add it to the "Backlog" and I will leave it open. Thank you for your contributions.

@stale stale bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label Feb 23, 2020
@stale
Copy link

stale bot commented Mar 1, 2020

Closing this issue after a prolonged period of inactivity. If this issue is still present in the latest release, please feel free to create a new issue with up-to-date information.

@stale stale bot closed this as completed Mar 1, 2020
@facebook facebook locked as resolved and limited conversation to collaborators Mar 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug Stale There has been a lack of activity on this issue and it may be closed soon.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants