Allow localhost database URL by rynobax · Pull Request #426 · firebase/firebase-js-sdk (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation10 Commits5 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
This change was discussed in #116.
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!) and we'll verify. Thanks.
- If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.
- If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot. The email used to register you as an authorized contributor must be the email used for the Git commit.
- In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.
This was referenced
Jan 11, 2018
| } else if (parts.length === 2) { |
|---|
| domain = parts[0]; |
| } else if (parts[0].split(':')[0].toLowerCase() === 'localhost') { |
| subdomain = 'localhost'; |
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.
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 Error: Invalid Firebase Database URL failed: First argument must be a valid firebase URL and the path can't contain ".", "#", "$", "[", or "]".
The error seems to be triggered by this line, because the namespace is an empty string:
| !isValidKey(parsedUrl.repoInfo.namespace) || | | --------------------------------------------- |
If I remove that line all the tests pass.
The two options that come to my mind are:
- Do not set the subdomain on localhost domains
- In validation.ts, skip the namepace validity check if the host is
localhost
Or:
- Set the subdomain on localhost domains
- Remove the extra check I added to the assert
Do not set the subdomain on localhost domains
In validation.ts, skip the namespace validity check if the host is localhost
Please go ahead and do this. It seems cleaner.
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.
urish added a commit to urish/firebase-server that referenced this pull request
