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

[all] Switch Android from api dependencies to implementation dependencies #39

Closed

Conversation

collinjackson
Copy link
Contributor

@collinjackson collinjackson commented Aug 26, 2019

Flutterfire plugins don't expose a public API to other Java libraries (with the exception of the plugin registration process). I am wondering if it would be reasonable to allow a developer to mix and match plugins that depend on different versions of Firebase Android SDK.

Using implementation dependencies instead of api dependencies gets our dependencies off the classpath of the developer's app and reduces the chance of Gradle build errors due to version resolution conflicts. Being able to mix and match Flutterfire plugins on Android might reduce the need for "symbolic" Dart packages.

Here's an article on the differences of api vs implementation in libraries:
https://docs.gradle.org/current/userguide/java_library_plugin.html#sec:java_library_separation

I think this might be a more ideal approach for the problem that is addressed by #12 where adding an explicit Firebase dependency to the developer's app-level build.gradle can lead to unexpected build breaks elsewhere.

Possible con: It might increase the size of builds slightly if developers are depending on multiple implementations of Firebase Android SDKs. However, I'm not seeing this in practice.

Related Issues

flutter/flutter#37837

@collinjackson collinjackson requested review from mklim and amirh August 26, 2019 17:32
@collinjackson collinjackson changed the title Switch Android from api dependencies to implementation dependencies [all] Switch Android from api dependencies to implementation dependencies Aug 26, 2019
@collinjackson collinjackson requested a review from kroikie August 26, 2019 17:46
Copy link

@mklim mklim left a comment

Choose a reason for hiding this comment

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

Gradle changes LG, but should there also be version bumps for all of these?

This is just a working theory, but I think there's a small chance that this may introduce compile failures where there weren't any before, in rare cases where a dev was relying on the api to expose something that they themselves were also depending on without naming it themselves like they should've. Not 100% on this because the way we glob plugins together for Flutter builds makes it kind of murky. But something to keep an eye on.

@collinjackson
Copy link
Contributor Author

Gradle changes LG, but should there also be version bumps for all of these?

Yeah, I'll bump the versions and update CHANGELOGs if you're on board with the approach.

This is just a working theory, but I think there's a small chance that this may introduce compile failures where there weren't any before, in rare cases where a dev was relying on the api to expose something that they themselves were also depending on without naming it themselves like they should've. Not 100% on this because the way we glob plugins together for Flutter builds makes it kind of murky. But something to keep an eye on.

Any ideas for how we can mitigate that risk? Release it as a breaking change? Migrate a few plugins and then do the rest?

@mklim
Copy link

mklim commented Aug 26, 2019

Any ideas for how we can mitigate that risk? Release it as a breaking change? Migrate a few plugins and then do the rest?

I think we could probably verify whether or not it's actually an issue locally by creating an app that uses one of the current api dependencies in its app's Java code, and seeing if it compiles first and then fails to compile with it changes to implementation.

If we see it breaking in a local test like that I think releasing it as a breaking change makes sense. If that test doesn't show this breaking, I think it's probably safe to release this. I alluded to this a little earlier but I think there's a chance that the way we glob all our plugins together when we build means that implementation vs api doesn't really practically matter much. I think it's unlikely, but I wouldn't be totally surprised if we saw apps still able to compile relying on plugin implementation dependencies somehow. @blasten may know for sure.

@collinjackson
Copy link
Contributor Author

I'm definitely seeing a difference between api and implementation in the sense that following the steps in #12 no longer results in the build failure:

  1. add implementation 'com.google.firebase:firebase-core:17.0.0' to app-level build.gradle
  2. add a dependency on firebase_messaging to pubspec.yaml
  3. flutter build apk

We can test for this in the all_plugins test probably. I've got that turned off for Flutterfire, but will enable it soon.

@collinjackson
Copy link
Contributor Author

collinjackson commented Aug 27, 2019

I spoke with Maurice and Arthur and per your suggestion I think we should release it as a breaking change. If developers are relying on the api dependencies to resolve their Java code's build dependencies successfully, then this change will cause their builds to fail. Without having source code for the developers' apps, I don't have any way to know how prevalent this situation is.

I'll go ahead and release this as a breaking change.

Copy link

@mklim mklim left a comment

Choose a reason for hiding this comment

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

LGTM

packages/cloud_firestore/CHANGELOG.md Outdated Show resolved Hide resolved
@kroikie kroikie force-pushed the api-to-implementation branch from 1599a8e to c1c4538 Compare November 8, 2019 21:47
@kroikie kroikie force-pushed the api-to-implementation branch from 4f1dfcf to 8702d49 Compare November 15, 2019 18:12
@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@kroikie
Copy link
Collaborator

kroikie commented Jun 30, 2020

We are switching to using BoM for managing Android dependencies so closing this.

@kroikie kroikie closed this Jun 30, 2020
@Salakar Salakar mentioned this pull request Jul 3, 2020
14 tasks
@firebase firebase locked and limited conversation to collaborators Aug 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants