Skip to content
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.

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

feat(fis): Adding the admin.installations() API for deleting Firebase installation IDs #1187

Merged
merged 10 commits into from
Jun 23, 2021

Conversation

hiranya911
Copy link
Contributor

@hiranya911 hiranya911 commented Mar 4, 2021

  • Added the new admin.installations() API.
  • Deprecated the existing admin.instanceId() API. The implementation of this API now just invokes the installations API under the hood.
  • The admin.instanceId() retains backward compatibility by converting the FirebaseInstallationsError raised by the new implementation to the old FirebaseInstanceIdError type. Error codes are the same, but the error message may defer.

go/firebase-admin-fis

RELEASE NOTE: Added a new admin.installations() API to replace the existing admin.instanceId() API. Developers are advised to migrate to the new API for deleting Firebase instance IDs and installation IDs.

@hiranya911 hiranya911 added the release:stage Stage a release candidate label Mar 4, 2021
Copy link
Member

@lahirumaramba lahirumaramba left a comment

Choose a reason for hiding this comment

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

LGTM. Left one comment.

src/installations/installations.ts Outdated Show resolved Hide resolved
@lahirumaramba lahirumaramba removed their assignment Mar 10, 2021
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.

Thanks Hiranya! Some nits and one link to update.

src/firebase-app.ts Outdated Show resolved Hide resolved
src/installations/index.ts Outdated Show resolved Hide resolved
src/instance-id/index.ts Outdated Show resolved Hide resolved
src/utils/error.ts Outdated Show resolved Hide resolved
test/unit/instance-id/instance-id.spec.ts Show resolved Hide resolved
@hiranya911
Copy link
Contributor Author

Thanks @egilmorez. I've responded to your comments. PTAL.

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.

LGTM, thanks Hiranya!

@hiranya911 hiranya911 removed the release:stage Stage a release candidate label Mar 18, 2021
@hiranya911 hiranya911 assigned hiranya911 and unassigned avolkovi Apr 12, 2021
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.

Here's a couple of things to look at Hiranya. Thanks!

section:
- title: "Installations"
path: /docs/reference/admin/node/admin.installations.Installations-1

- title: "admin.instanceId"
Copy link
Contributor

Choose a reason for hiding this comment

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

It's possible to add a "status: deprecated" flag here to create this kind of effect:

https://firebase.google.com/docs/reference/android/com/google/firebase/iid/package-summary?hl=en

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What does that look like in the yaml? Can you point me to an example?

*```
*
* @param app Optional app whose `Installations` service to
* return. If not provided, the default `Installations` service will be
Copy link
Contributor

@egilmorez egilmorez Apr 30, 2021

Choose a reason for hiding this comment

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

"Optional app whose Installations service to

  • return." makes sense but isn't 100% grammatical.

"Optional app for which to return the Installations service" is valid but not great.

"Optional app whose Installations service should be

  • returned." sounds better, but "should" seems weak.

I'll poll the tech writers and see who has a solution :)

src/installations/index.ts Show resolved Hide resolved
src/installations/index.ts Outdated Show resolved Hide resolved
@hiranya911 hiranya911 merged commit 0f9a7de into master Jun 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants