Skip to content

add FIREBASE_DATABASE_EMULATOR_HOST_VAR - #596

Merged
jmwski merged 6 commits into
masterfrom
wyszynski/rtdb-emulator-envvar
Jul 19, 2019
Merged

add FIREBASE_DATABASE_EMULATOR_HOST_VAR#596
jmwski merged 6 commits into
masterfrom
wyszynski/rtdb-emulator-envvar

Conversation

@jmwski

@jmwskijmwski commented Jul 18, 2019•
edited
Loading

Copy link
Copy Markdown
Contributor

Support specifying database endpoint with FIREBASE_DATABASE_EMULATOR_HOST.

@jmwski
jmwski requested a review from samtsternJuly 18, 2019 23:45
samtstern
samtstern approved these changes Jul 18, 2019
View reviewed changes

@samtsternsamtstern left a comment

Copy link
Copy Markdown
Contributor

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

@hiranya911hiranya911 left a comment

Copy link
Copy Markdown
Contributor

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.

Comment threadtest/unit/firebase-app.spec.ts Outdated
Comment threadtest/unit/firebase.spec.ts Outdated
Comment threadtest/unit/firebase-app.spec.ts Outdated
@jmwski
jmwski requested a review from hiranya911July 19, 2019 00:55
@jmwski

jmwski commented Jul 19, 2019

Copy link
Copy Markdown
ContributorAuthor

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

@hiranya911hiranya911 left a comment

Copy link
Copy Markdown
Contributor

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.

Comment threadpackage.json Outdated
@jmwskijmwski mentioned this pull request Jul 19, 2019
Closed
egilmorez
egilmorez reviewed Jul 19, 2019
View reviewed changes
Comment threadsrc/database/database.ts Outdated
egilmorez
egilmorez approved these changes Jul 19, 2019
View reviewed changes

@egilmorezegilmorez left a comment

Copy link
Copy Markdown
Contributor

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

@hiranya911hiranya911 left a comment

Copy link
Copy Markdown
Contributor

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 masterJul 19, 2019
@jmwski
jmwski deleted the wyszynski/rtdb-emulator-envvar branch July 19, 2019 21:53
@jmwskijmwski 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 freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

3 more reviewers
@hiranya911hiranya911hiranya911 approved these changes
@egilmorezegilmorezegilmorez approved these changes
@samtsternsamtsternsamtstern approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@jmwskijmwski

@samtsternsamtstern

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