Implemented IID Delete API #142
Conversation
| const FIREBASE_IID_TIMEOUT = 10000; | ||
|
|
||
| /** | ||
| * Class that provides mechanism to send requests to the Firebase Auth backend endpoints. |
There was a problem hiding this comment.
Update comment: Firebase IID backend endpoints.
src/instance-id/instance-id.ts
Outdated
| * Internals of an InstanceId service instance. | ||
| */ | ||
| class InstanceIdInternals implements FirebaseServiceInternalsInterface { | ||
| /** |
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
| * @return {Promise<()>} An empty Promise that will be fulfilled when the service is deleted. | ||
| */ | ||
| public delete(): Promise<void> { | ||
| // There are no resources to clean up |
src/instance-id/instance-id.ts
Outdated
| } | ||
|
|
||
| export class InstanceId implements FirebaseServiceInterface { | ||
| public INTERNAL: InstanceIdInternals = new InstanceIdInternals(); |
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
| export class InstanceId implements FirebaseServiceInterface { | ||
| public INTERNAL: InstanceIdInternals = new InstanceIdInternals(); | ||
|
|
||
| private app_: FirebaseApp; |
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);
| chai.use(chaiAsPromised); | ||
|
|
||
| describe('InstanceId', () => { | ||
| let iid: InstanceId; |
|
|
||
| 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.
| }); | ||
|
|
||
| it('should be rejected given an invalid instance ID', () => { | ||
| return iid.deleteInstanceId('') |
There was a problem hiding this comment.
Why don't you use should.eventually.be.rejected.and.have... like above
| }); | ||
|
|
||
| it('should throw an error when the backend returns an error', () => { | ||
| // Stub getAccountInfoByUid to throw a backend error. |
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.
A new API that enables deleting instance IDs associated with a Firebase project.
go/firebase-admin-iid