Skip to content

feat(rtdb): Support RTDB Emulator via FIREBASE_DATABASE_EMULATOR_HOST. #313

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

Merged
merged 11 commits into from
Aug 1, 2019

Conversation

yuchenshi
Copy link
Member

@yuchenshi yuchenshi commented Jul 26, 2019

Discussion

(Discussed internally.)

Testing

I've manually ran the integration tests with and without the emulator and verified that they pass.
We may consider running the emulator-based integration test on Travis in a follow up PR.

API Changes

No public API change except FakeCredential and the FIREBASE_DATABASE_EMULATOR_HOST env var.

RELEASE NOTE: Developers can now test their {{database}} API calls by directing the SDK traffic to the RTDB emulator. Set the FIREBASE_DATABASE_EMULATOR_HOST environment variable to specify the emulator endpoint in host:port format.

Copy link
Contributor

@hiranya911 hiranya911 left a comment

Choose a reason for hiding this comment

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

Thanks @yuchenshi for implementing this. It looks pretty good. I added a few suggestions on how the code can be cleaned up a bit.

@hiranya911 hiranya911 assigned yuchenshi and unassigned hiranya911 Jul 27, 2019
@yuchenshi yuchenshi assigned hiranya911 and unassigned yuchenshi Jul 31, 2019
@yuchenshi yuchenshi requested a review from hiranya911 July 31, 2019 22:53
Copy link
Contributor

@hiranya911 hiranya911 left a comment

Choose a reason for hiding this comment

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

Looks pretty good. I've pointed out a few things that can be done to make the code a bit more clear and hopefully simpler.

return base_url, {'ns': namespace}, use_fake_creds

@classmethod
def _parse_production_url(cls, parsed_url):
Copy link
Contributor

Choose a reason for hiding this comment

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

Make this and _parse_db_url instance methods. Then you don't need to pass emulator_host around. It can be read from self.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, but no, thanks. I just tried that and it made testing a nightmare. I don't want to initialize a full _DatabaseService class with all the options and monkey-patch os.environ just to test the parsing logic.

@hiranya911 hiranya911 assigned yuchenshi and unassigned hiranya911 Aug 1, 2019
@yuchenshi yuchenshi requested a review from hiranya911 August 1, 2019 01:19
@yuchenshi yuchenshi assigned hiranya911 and unassigned yuchenshi Aug 1, 2019
@yuchenshi yuchenshi requested a review from hiranya911 August 1, 2019 18:25
Copy link
Contributor

@hiranya911 hiranya911 left a comment

Choose a reason for hiding this comment

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

Thanks @yuchenshi. Looks pretty good. Just a couple of nits and questions. We can merge once they are resolved.

@hiranya911 hiranya911 assigned yuchenshi and unassigned hiranya911 Aug 1, 2019
@yuchenshi yuchenshi requested a review from hiranya911 August 1, 2019 20:40
@yuchenshi yuchenshi assigned hiranya911 and unassigned yuchenshi Aug 1, 2019
Copy link
Contributor

@hiranya911 hiranya911 left a comment

Choose a reason for hiding this comment

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

Thanks. LGTM 👍

@hiranya911 hiranya911 assigned yuchenshi and unassigned hiranya911 Aug 1, 2019
@yuchenshi yuchenshi merged commit aecd35e into master Aug 1, 2019
@yuchenshi yuchenshi deleted the ys/rtdb-emulator branch August 1, 2019 21:22
@hiranya911 hiranya911 changed the title Support RTDB Emulator via FIREBASE_DATABASE_EMULATOR_HOST. feat(rtdb): Support RTDB Emulator via FIREBASE_DATABASE_EMULATOR_HOST. Aug 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants