Static Loading of Service Namespaces#106
Conversation
jshcrowthe
left a comment
jshcrowthe
left a comment
There was a problem hiding this comment.
Seems pretty good, if you'd take a look at the few comments I had that'd be great.
| ApplicationDefaultCredential, | ||
| } from './auth/credential'; | ||
| import {Firestore} from '@google-cloud/firestore'; | ||
| import * as _ from 'lodash/object'; |
There was a problem hiding this comment.
AFAICT you are bringing this in to do:
_.assign()There is a native JS version of this called Object.assign, it may be able to trim a dependency for you.
There was a problem hiding this comment.
Brilliant! Done. Thanks for the tip.
| }; | ||
| const reference = admin.firestore().collection('cities').doc() | ||
| return reference.set(expected) | ||
| return Promise.resolve() |
There was a problem hiding this comment.
What was the rationale behind this change?
It seems like you've added an extra step in the chain unnecessarily.
There was a problem hiding this comment.
This is to allow any errors thrown by admin.firestore.FieldValue to be caught in the catch block. I noticed that errors thrown outside the promise chain, cause the test suite to come to a halt.
There was a problem hiding this comment.
Ohhh gotcha. Ok, then yeah this is fine then.
phra
left a comment
phra
left a comment
There was a problem hiding this comment.
using a getter function breaks mocks depending on es6 module import.. ref: https://railsware.com/blog/2017/01/10/mocking-es6-module-import-without-dependency-injection/
| AppErrorCodes.INTERNAL_ERROR, | ||
| 'INTERNAL ASSERT FAILED: Firebase auth() service has not been registered.', | ||
| ); | ||
| get auth(): FirebaseServiceNamespace<Auth> { |
4 participants
Admin SDK monkeypatches service namespaces (
Auth,Messagingetc.) to the parentadminnamespace at SDK initialization. Theindex.tsfile calls a set ofregister*()functions, which imports the services, and attaches them toadmin. However, for most services this both unnecessary, and error prone.This PR gets rid of the monkeypatching for all services but RTDB. There are several benefits for this change:
adminnamespace.adminin a way that is both simple and type safe. Services will continue to be loaded/initialized on-demand (e.g.Authservice will not get initialized untiladmin.auth()is invoked).adminnamespace can be verified with unit tests.adminnamespace without having them implement theFirebaseServiceInterface. This is useful when exposing external types such asFirestorefrom theadminnamespace.