Skip to content

Conversation

samtstern
Copy link
Contributor

@hiranya911 before I go any farther:

  • Was there a reason getCertificate() is not in the .d.ts?
  • Are these typedef files normally hand-written?
  • Many implementations return null from getCertificate() get the typedef says Certificate not Certificate | null or Certificate | undefined ... why does that even compile?

@samtstern samtstern requested a review from hiranya911 June 18, 2019 21:33
@hiranya911
Copy link
Contributor

Hi @samtstern.

  1. Certificate is not part of our public API. So anything related to it doesn't appear in the typings.
  2. Yes. Both Admin SDK's and JS SDK's typings are hand-written.
  3. That's not something TypeScript enforces by default. You have to explicitly request it with --strictNullChecks.

@samtstern
Copy link
Contributor Author

So the reason I think we should expose this (I may misunderstand):

  • You can pass the credential field when you do admin.initializeApp() and we expect an admin.credential.Credential
  • If you do this and then use Firestore, it will explode because it calls getCertificate() on the credential object which is not guaranteed by the interface. See code for details

So if credential is part of our public API then I think the API should express all of the properties and methods you need to make sure nothing crashes.

@hiranya911
Copy link
Contributor

Then we ought to fix that in the admin Firestore code. Let's not add "Certificate" to the public API. For one thing it's a terrible (and incorrect) name. I've let it remain in the code because it's considered an internal API. Secondly there's no equivalent concept in any of the other GCP libraries.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants