Skip to content

Added firebase-installations #257

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 2 commits into from
Mar 19, 2022
Merged

Added firebase-installations #257

merged 2 commits into from
Mar 19, 2022

Conversation

suntrix
Copy link
Contributor

@suntrix suntrix commented Nov 23, 2021

No description provided.

@suntrix suntrix changed the title Installations Added firebase-installations Nov 24, 2021
@suntrix
Copy link
Contributor Author

suntrix commented Nov 30, 2021

@nbransby / @Reedyuk can you take a look?

@nbransby
Copy link
Member

@suntrix you are contributor #1! 🥇

Isn't installations just part of firebase-app? Would it not be better to add the API there instead of a separate module?

@suntrix
Copy link
Contributor Author

suntrix commented Dec 13, 2021

@suntrix you are contributor #1! 🥇

Isn't installations just part of firebase-app? Would it not be better to add the API there instead of a separate module?

🥳

I initially did that, but then I noticed they have a separate namespace for it & moved it to separate module.
TBH I'm good with either, I can move it back if you'd prefer that.

@nbransby
Copy link
Member

namespace as in package name in android? Yes I think its best to move it back as there is nothing to be gained from separate package (no reduction in dependencies if not using the installations api)


val androidMain by getting {
dependencies {
api("com.google.firebase:firebase-installations")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nbransby Actually there's a separate package for Android, so probably makes sense to leave it as separate module.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nbransby Can you take a look? I have some more PRs in my pipeline :)

@suntrix
Copy link
Contributor Author

suntrix commented Jan 26, 2022

@nbransby any possibility to merge it this week?

@nbransby nbransby merged commit 097f548 into GitLiveApp:master Mar 19, 2022
@suntrix suntrix deleted the installations branch May 14, 2022 16:19
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants