Skip to content

add FIREBASE_DATABASE_EMULATOR_HOST_VAR #596

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Jul 19, 2019
Merged

add FIREBASE_DATABASE_EMULATOR_HOST_VAR #596

merged 6 commits into from
Jul 19, 2019

Conversation

jmwski
Copy link
Contributor

@jmwski jmwski commented Jul 18, 2019 •
edited
Loading

Support specifying database endpoint with FIREBASE_DATABASE_EMULATOR_HOST.

@jmwski jmwski requested a review from samtstern July 18, 2019 23:45
samtstern
samtstern approved these changes Jul 18, 2019
View reviewed changes
Copy link
Contributor

@samtstern samtstern left a comment

Choose a reason for hiding this comment

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

@hiranya911 Jan and I did a bunch of manual testing today, this simplified version will work in conjunction with some changes to the emulator itself (which are also out for review)

hiranya911
hiranya911 suggested changes Jul 19, 2019
View reviewed changes
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 @IanWyszynski. Looks pretty good. Just have some comments about the tests.

@jmwski jmwski requested a review from hiranya911 July 19, 2019 00:55
@jmwski
Copy link
Contributor Author

jmwski commented Jul 19, 2019

This is currently blocked on the minor version bump in firebase/firebase-js-sdk#2005, which must get merged first.

hiranya911
hiranya911 suggested changes Jul 19, 2019
View reviewed changes
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 @IanWyszynski. Code looks good. Please push the updated package-lock file so the tests can pass. Then I can merge.

@jmwski jmwski mentioned this pull request Jul 19, 2019
Closed
egilmorez
egilmorez reviewed Jul 19, 2019
View reviewed changes
egilmorez
egilmorez approved these changes Jul 19, 2019
View reviewed changes
Copy link
Contributor

@egilmorez egilmorez left a comment

Choose a reason for hiding this comment

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

Other than a tiny nit, comments have my blessing for style. Thanks!

hiranya911
hiranya911 approved these changes Jul 19, 2019
View reviewed changes
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.

LGTM 👍

@jmwski jmwski merged commit e963005 into master Jul 19, 2019
@jmwski jmwski deleted the wyszynski/rtdb-emulator-envvar branch July 19, 2019 21:53
@jmwski jmwski mentioned this pull request Jul 22, 2019
Merged
jmwski added a commit that referenced this pull request Jul 23, 2019
@jmwski
Revert "add FIREBASE_DATABASE_EMULATOR_HOST_VAR (#596)"
a4ad828 
This reverts commit e963005.
jmwski added a commit that referenced this pull request Jul 23, 2019
@jmwski
Revert "add FIREBASE_DATABASE_EMULATOR_HOST_VAR (#596)" (#602)
5746a5c 
This reverts commit e963005.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hiranya911 hiranya911 hiranya911 approved these changes

@egilmorez egilmorez egilmorez approved these changes

@samtstern samtstern samtstern approved these changes

Assignees

@jmwski jmwski

@samtstern samtstern

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jmwski @hiranya911 @egilmorez @samtstern