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

Support the RTDB emulator #3491

Merged
merged 6 commits into from Aug 5, 2019
Merged

Support the RTDB emulator #3491

merged 6 commits into from Aug 5, 2019

Conversation

schmidt-sebastian
Copy link
Contributor

@schmidt-sebastian schmidt-sebastian commented Aug 1, 2019

This adds support for the emulator and also for the "ns" query param.\

Technically, iOS doesn't need the "ns" query param since "localhost" is a valid RTDB namespace name and we could just use "localhost" as the namespace. But since I just went all out on Android (firebase/firebase-android-sdk#680), I decided to make this PR equally awesome. Plus who doesn't like some good URL decoding/encoding mess.

@yuchenshi
Copy link
Member

@yuchenshi yuchenshi commented Aug 1, 2019

We still need people to be able to specify the database name (for example to test triggers), so just accepting localhost is not a great answer. So, thanks for the PR and it's really important!

Copy link
Contributor

@mikelehen mikelehen left a comment

LGTM with nits.

to the emulator, specify "http://<emulator_host>/" as your Database URL
(via `Database.database(url:)`).
If you refer to your emulator host by IP rather than by domain name, you may
also need to specify a namespace ("http://<emulator_host>/?ns=<project_id>").
Copy link
Contributor

@mikelehen mikelehen Aug 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"namespace" => " project id" ? Or some other implementation-independent term.

Copy link
Contributor Author

@schmidt-sebastian schmidt-sebastian Aug 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

bool secure;

if (urlComponents.port != nil) {
secure = [urlComponents.scheme isEqualToString:@"https"];
Copy link
Contributor

@mikelehen mikelehen Aug 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, on web I think we support wss:// URLs (which IIRC disables long-polling). I wonder if we should try for some consistency.

Copy link
Contributor Author

@schmidt-sebastian schmidt-sebastian Aug 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added support for "wss".

secure = [urlComponents.scheme isEqualToString:@"https"];
host = [host stringByAppendingFormat:@":%@", urlComponents.port];
} else if ( [urlComponents.host isEqualToString:@"localhost"]) {
secure = NO;
Copy link
Contributor

@mikelehen mikelehen Aug 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we do this? Did the old code do this?

Copy link
Contributor Author

@schmidt-sebastian schmidt-sebastian Aug 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like this is not winning anyone over, so I dropped this from the iOS port (will drop the special casing for Android too).

}


NSString* pathString = [self decodePath:[NSString stringWithFormat:@"/%@", originalPathString]];
FPath * path = [[FPath alloc] initWith:pathString];
Copy link
Contributor

@mikelehen mikelehen Aug 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems like there are extra spaces here? Why does clang-format not fix that?

Copy link
Contributor Author

@schmidt-sebastian schmidt-sebastian Aug 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed manually. Seems like we are excluded from the formatter (but it still takes 10 seconds to run). I'll get the formatting enabled for RTDB in a follow up.

Copy link
Contributor Author

@schmidt-sebastian schmidt-sebastian Aug 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This source is now clang-formatted.

Copy link
Contributor Author

@schmidt-sebastian schmidt-sebastian left a comment

Comments addressed.

to the emulator, specify "http://<emulator_host>/" as your Database URL
(via `Database.database(url:)`).
If you refer to your emulator host by IP rather than by domain name, you may
also need to specify a namespace ("http://<emulator_host>/?ns=<project_id>").
Copy link
Contributor Author

@schmidt-sebastian schmidt-sebastian Aug 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

bool secure;

if (urlComponents.port != nil) {
secure = [urlComponents.scheme isEqualToString:@"https"];
Copy link
Contributor Author

@schmidt-sebastian schmidt-sebastian Aug 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added support for "wss".

secure = [urlComponents.scheme isEqualToString:@"https"];
host = [host stringByAppendingFormat:@":%@", urlComponents.port];
} else if ( [urlComponents.host isEqualToString:@"localhost"]) {
secure = NO;
Copy link
Contributor Author

@schmidt-sebastian schmidt-sebastian Aug 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like this is not winning anyone over, so I dropped this from the iOS port (will drop the special casing for Android too).

}


NSString* pathString = [self decodePath:[NSString stringWithFormat:@"/%@", originalPathString]];
FPath * path = [[FPath alloc] initWith:pathString];
Copy link
Contributor Author

@schmidt-sebastian schmidt-sebastian Aug 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed manually. Seems like we are excluded from the formatter (but it still takes 10 seconds to run). I'll get the formatting enabled for RTDB in a follow up.

@schmidt-sebastian schmidt-sebastian merged commit c706ce9 into master Aug 5, 2019
2 checks passed
@schmidt-sebastian schmidt-sebastian deleted the mrschmidt/emulator branch Aug 5, 2019
@firebase firebase locked and limited conversation to collaborators Oct 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants