Conversation
bojeil-google
left a comment
bojeil-google
left a comment
There was a problem hiding this comment.
Looks good! Mostly nits.
There was a problem hiding this comment.
Update comment: Firebase IID backend endpoints.
src/instance-id/instance-id.ts
Outdated
There was a problem hiding this comment.
Update the indentation to be consistent. We use 2 spaces in this case.
src/instance-id/instance-id.ts
Outdated
src/instance-id/instance-id.ts
Outdated
There was a problem hiding this comment.
Indentation needs to be fixed here too and the rest of the class.
src/instance-id/instance-id.ts
Outdated
There was a problem hiding this comment.
Let's stick with no trailing _ for private variables. This is also the recommended style for ts.
There was a problem hiding this comment.
That is going to conflict with the get app() getter. Is it better to remove that, and expose app as a public attribute of this class? But I guess that would make app writable, which we don't want.
| it('should return a valid namespace when the named app is initialized', () => { | ||
| let app: FirebaseApp = firebaseNamespace.initializeApp(mocks.appOptions, 'testApp'); | ||
| let iid: InstanceId = firebaseNamespace.instanceId(app); | ||
| expect(iid).to.not.be.null; |
There was a problem hiding this comment.
Add check:
expect(iid.app).to.be.deep.equal(app);
|
|
||
| it('should be rejected given no instance ID', () => { | ||
| return (iid as any).deleteInstanceId() | ||
| .should.eventually.be.rejected.and.have.property('code', 'instance-id/invalid-instance-id'); |
There was a problem hiding this comment.
Indent second line .should... and .then below too.
There was a problem hiding this comment.
Why don't you use should.eventually.be.rejected.and.have... like above
There was a problem hiding this comment.
Update comment getAccountInfoByUid
|
Made the suggested changes, and left one question. Over to @bojeil-google for another look. |
|
@bojeil-google I pushed a couple of commits to improve error handling and test coverage. PTAL. |
…mantics of util.format()
| }); | ||
| }); | ||
|
|
||
| describe('deleteInstanceId', () => { |
There was a problem hiding this comment.
Looks good. One suggestion: i think it would be a good idea to also confirm the error code too.
…o be consistent across all SDKs.
2 participants
A new API that enables deleting instance IDs associated with a Firebase project.
go/firebase-admin-iid