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

Updated types to include potential for other keys in AppOptions. #148

Merged
merged 2 commits into from
Dec 1, 2017

Conversation

bmass02
Copy link
Contributor

@bmass02 bmass02 commented Nov 30, 2017

Discussion

Since the updates to Firebase Functions (firebase/firebase-functions#127, firebase/firebase-functions#138) config() function, it seems the AppOptions type needs to be updated to correctly reflect what keys might be present on the object. I might be missing some (not sure exactly what keys might be present), but I added those that I've used previously (worked previously because config().firebase used to be of type any).

API Changes

Only changes to the type definitions.

@hiranya911
Copy link
Contributor

Thanks @bmass02 for the PR. authDomain is not an option supported in Admin SDK. So it's a little weird to include it in our API definitions. Good call on projectId though.

@laurenzlong @bojeil-google any thoughts on how this should be handled?

@bmass02
Copy link
Contributor Author

bmass02 commented Nov 30, 2017

@hiranya911 Interesting. I currently use authDomain in Cloud Functions to provide links to the correctly deployed project. I can always just use projectId and add the extra text that creates authDomain, but it just seemed easier to use authDomain since it already existed.

@hiranya911
Copy link
Contributor

@bmass02 Thanks for clarifying.

@laurenzlong What does Functions SDK use authDomain for? Does it have any usage in a backend framework like Cloud Functions?

@laurenzlong
Copy link

@hiranya911 authDomain not actually used for anything (I had mistakenly thought that it was used by the admin SDK, but it turns out I got it confused with the client SDKs). One can argue that it should be removed from functions.config() (along with apiKey, since all Google APIs work within the Functions Runtime without needing an API key).

@laurenzlong
Copy link

@bmass02 What do you mean by "links to the correctly deployed project"?

@bojeil-google
Copy link
Contributor

I don't see any current need for authDomain or apiKey for the Admin SDK or Cloud Functions. I would recommend removing them from functions.config().

@bmass02
Copy link
Contributor Author

bmass02 commented Nov 30, 2017

I have different Firebase projects for different environments, so I use the authDomain to update links (sent via text/email). I also use it for CORS on HTTPS functions that use Express. It honestly isn't a big deal. I was just going to update the type definitions to meet the potential scenarios that might be encountered. I'll go ahead and switch to using projectId.

@hiranya911
Copy link
Contributor

Thanks all for the input. @bmass02 if you can drop authDomain from this PR, I will be happy to merge it. Not specifying projectId in our typings was an oversight, and we should fix that.

@laurenzlong I'll create an issue to remove authDomain and apiKey from functions.config(), so you can consider it for a future release.

@laurenzlong
Copy link

Great thanks.

Copy link
Contributor

@hiranya911 hiranya911 left a comment

Choose a reason for hiding this comment

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

LGTM 👍

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.

4 participants