-
Notifications
You must be signed in to change notification settings - Fork 893
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
Allow localhost database URL #426
Allow localhost database URL #426
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
I signed it! |
CLAs look good, thanks! |
@@ -131,6 +131,8 @@ export const parseURL = function( | |||
subdomain = parts[0].toLowerCase(); | |||
} else if (parts.length === 2) { | |||
domain = parts[0]; | |||
} else if (parts[0].split(':')[0].toLowerCase() === 'localhost') { | |||
subdomain = 'localhost'; |
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.
Can you move the port extraction logic from line 136 up so that it happens before this if-statement? Then we won't have to double parse the port number.
Then I would also suggest that instead of assigning 'localhost' to subdomain
you should assign it to domain
. A subdomain would be something like 'foo' in 'foo.localhost'.
I believe I made the change you requested about parsing the port number, let me know if that is not what you had in mind. I also changed it to set the domain to localhost. However, the subdomain also needs to be set to something, as it is used for the 'namespace' variable, which cannot be empty. For now it also sets the subdomain to 'localhost', as I am not entirely sure of what the namespace is used for. |
Thanks, the changes look mostly good. The namespace is used by our backend if you are not connecting the intended host. For mocking, this should not be needed. It seems like you changed the assert to no longer fire AND you set the subdomain to localhost? Can you explain why we need to do both - otherwise I would just remove the setting of the subdomain and we should be good to submit this PR. |
If the subdomain is not set, the test fails with the error The error seems to be triggered by this line, because the namespace is an empty string:
If I remove that line all the tests pass. The two options that come to my mind are:
Or:
|
Please go ahead and do this. It seems cleaner. |
I made the changes. |
LGTM. Note that this won't work for localhost URLs that don't have a port set. I will send you a follow up PR to add support for this. |
* Allow localhost database URL * Setting domain on localhost server address * [AUTOMATED]: Prettier Code Styling * Allow namespace to be invalid if the host is localhost * [AUTOMATED]: Prettier Code Styling
it's no longer requires, thanks to firebase/firebase-js-sdk#426
This change was discussed in #116.