[firebase_dynamic_links] support v2 embedding #1372

bparrishMines · 2019-11-05T22:46:11Z

Description

… dynamic_embedding

xster · 2019-11-12T22:37:48Z

packages/firebase_dynamic_links/android/build.gradle

+        android {
+            dependencies {
+                def lifecycle_version = "1.1.1"
+                compileOnly "android.arch.lifecycle:runtime:$lifecycle_version"


are there existing androidx transitive dependencies? (since there's an androidx annotation already) Is this a downgrade?

These use the latest instruction from the migration wiki: https://github.com/flutter/flutter/wiki/Experimental:-Create-Flutter-Plugin.

Quoting: flutter/plugins#2221 (comment)
We introduced a constraint that the app uses the plugin has to be migrated to androidx. This patch removes the constraint.

@mklim @blasten we had this conversation separately too. I guess we're saying mixing is ok and we're just letting plugins build as aars as fallbacks?

xster · 2019-11-12T22:40:23Z

...ndroid/src/main/java/io/flutter/plugins/firebasedynamiclinks/FirebaseDynamicLinksPlugin.java

+    implements FlutterPlugin, ActivityAware, MethodCallHandler, NewIntentListener {
+  private Registrar registrar;
+  private MethodChannel channel;
+  private ActivityPluginBinding activityBinding;

  private FirebaseDynamicLinksPlugin(Registrar registrar, MethodChannel channel) {
    this.registrar = registrar;
    this.channel = channel;


You don't have to pass this in right? The less potentially dangling nulls there are (depending on the path you take), the less likely there will be accidental null pointer exceptions

I should need it. The FirebaseDynamicLinks plugins makes callbacks to dart: https://github.com/FirebaseExtended/flutterfire/pull/1372/files#diff-3f6c424218a2662c8ddc4b322f30bf66R81.

I could hold on to a BinaryMessenger and instantiate a MethodChannel every time we want to call invokeMethod, but I think this looks cleaner.

xster · 2019-11-12T22:42:56Z

...ndroid/src/main/java/io/flutter/plugins/firebasedynamiclinks/FirebaseDynamicLinksPlugin.java

+    implements FlutterPlugin, ActivityAware, MethodCallHandler, NewIntentListener {
+  private Registrar registrar;
+  private MethodChannel channel;
+  private ActivityPluginBinding activityBinding;

  private FirebaseDynamicLinksPlugin(Registrar registrar, MethodChannel channel) {


You added annotations right? Tag Registrar nonnull. Even better, since you only pull the activity out of the registrar, just change the instance variable to activity so future maintainers don't accidentally do registrar.messenger() and expand the encapsulation in the future and trip on a null half of the time.

I removed the annotations in this PR because the ecosystem team decided that we won't include androidx in any plugin that doesn't require it. It's so that a Flutter app that doesn't include androidx can use this plugin.

However, this is a good point about someone accidently using the registrar. What about adding the @Deprecated annotation with an explanation about registrar remaining until we decide to only support v2 embedding flutter apps.

This does remind me that I also need to add documentation for the plugin now. Updated PR

Sorry, this tab was open and I missed the new comments. I agree with Maurice's summary about annotation, commented here.

For @Deprecated, I vaguely remember floating the idea by @amirh offline and he was against it. If I'm remembering right the reason being was that for now the static registrar registerWith method is fully supported, and we don't want to flag it as deprecated before we're actually really deprecating it. I think those uses will show up as compile time warnings too, so probably better not to bring them in.

We don't want to @Deprecate the actual Registrar API in the engine like @mklim said.
If you meant @Deprecate the instance variable, that sounds ok. I don't have a strong opinion. I was just making a general comment (regardless of what the class is) to minimize access. If your plugin instance wants an activity, just keep an activity and not its provider.

I agree. I remember that we originally kept an instance of Registrar and not Activity because the method Registrar.getActivity() would sometimes return different activities in add-to-app scenarios.

Since we're now migrating to v2 embedding, I don't this this would be the case anymore. I updated the plugin to hold a reference to Activity.

xster · 2019-11-12T22:43:41Z

...ndroid/src/main/java/io/flutter/plugins/firebasedynamiclinks/FirebaseDynamicLinksPlugin.java

  private OnCompleteListener<ShortDynamicLink> createShortLinkListener(final Result result) {
    return new OnCompleteListener<ShortDynamicLink>() {
      @Override
-      public void onComplete(@NonNull Task<ShortDynamicLink> task) {


Were these causing issues?

answered in #1372 (comment)

xster · 2019-11-12T22:44:26Z

...ndroid/src/main/java/io/flutter/plugins/firebasedynamiclinks/FirebaseDynamicLinksPlugin.java

 import android.content.Intent;
 import android.net.Uri;
-import androidx.annotation.NonNull;


You went from androidx to android.arch in the gradle, which seems to cause this to not work. Though I'm not sure if it's intentional. Or did firebase already conceptually "upgrade" to androidx @mklim

answered in #1372 (comment)

The latest thing we've talked about here is:

android.arch for the magic lifecycle dependencies in the Gradle script. That way Jetifier can run on the artifacts in cases where it's needed, and it will resolve as-is for unmigrated apps.

annotation should just be removed from both Gradle and source code files entirely. If it's present in source code Jetifier won't be able to run on it and it will cause compile failures for when there's AndroidX incompatibilities. I don't think that's worth just having the annotations.

If this is being stripped from the source code but it's still in the Gradle files as android.arch.annotation it should probably be removed from the Gradle files too. If it's not in the Gradle files at all this makes sense to me and shouldn't be causing known issues.

xster · 2019-11-12T22:45:01Z

LGTM

… dynamic_embedding

xster · 2019-11-21T04:08:54Z

LGTM

* bump core version to 0.4.1+4 (firebase#1395) * Remove deprecated firebase-core dependency (firebase#1417) * Remove deprecated firebase-core dependency * [firebase_auth] getIdToken use actual refresh value instead of checking if object exists (firebase#1334) * getIdToken use actual refresh value instead of checking if object exists * getIdToken refresh integration test * update xcode to v11 (firebase#1420) * Update FirebaseApp creation in tests (firebase#1406) * [firebase_database] Support v2 android embedder. (firebase#287) * [firebase_remote_config] Support v2 android embedder. (firebase#282) * [CI] Add version to example apps (firebase#1426) * Add hardcode version to example apps * [firebase_core] Add platform interface (firebase#1324) * Move firebase_core to firebase_core/firebase_core * Add firebase_core_platform_interface package * Update cirrus scripts and documentation * [firebase_dynamic_links] Add support of expanding from short links (firebase#1381) * Fixes strict compilation errors. (firebase#1331) * Upgrade crashlytics to v2 plugin API (firebase#1370) * Upgraded Crashlytics to v2 of Flutter Plugins. * Format documentation lists (See also & Errors) correctly (firebase#298) Fix documentation list format. * [firebase_auth] Fix NoSuchMethodError exception in `reauthenticateWithCredential` (firebase#237) * [firebase_auth] Fix NoSuchMethodError exception Fix NoSuchMethodError exception when calling FirebaseUser.reauthenticateWithCredential * add integration test * Fix analyzer warnings Co-Authored-By: Collin Jackson <[email protected]> * [cloud_firestore] [firebase_performance] fix analyzer warnings (firebase#1453) * fix analyzer warnings * [CI] Skip flaky firebase_performance driver tests (firebase#1452) * Skip flaky firebase_performance driver tests * [firebase_auth] Added missing Exception to the reauthenticateWithCredential docs (firebase#1448) (firebase#1449) * Added missing Exception to the reauthenticateWithCredential docs (firebase#1448) * [firebase_remote_config] Bumps Android Firebase dependency to 19.0.3 (firebase#1443) * Bump AGP, Gradle & Google Services Plugin * Bumps dependency to Firebase Config 19.0.3 * Replaces a deprecated method usage with the updated version * [firebase_messaging] Use UNUserNotificationCenter for ios 10+ (firebase#121) * Use UNUserNotificationCenter for ios 10+ * Add swift documentation * Move http test to README * Update CONTRIBUTING.md (firebase#1473) Removes obsolete recommendations about removing mockito (which is now used in testing federated plugins). * [firebase_core] Migrate to platform_interface (firebase#1472) * [firebase_messaging] Add an ArgumentError when passing invalid background message (firebase#252) * [firebase_dynamic_links] support v2 embedding (firebase#1372) * [cloud_firestore] Fix test that used FirebaseApp.channel (firebase#1476) * [cloud_firestore] Fix test that used FirebaseApp.channel * dartfmt * bump version * Fixed dynamic issue * Fixed dynamic issue * FIxed Object to list of object mapping * Fixed text result expectation * Reverted some changes * Satisfying formatter * Added android x to pass the test * Fixed changed log auto formatting

bparrishMines added 4 commits October 23, 2019 14:55

Implement FlutterPlugin and ActivityAware

0fde000

Merge branch 'master' of github.com:FirebaseExtended/flutterfire into…

804dd03

… dynamic_embedding

Setup tests

2407dc9

Add e2e tests to MainActivity

8f46c64

googlebot added the cla: yes label Nov 5, 2019

bparrishMines added 2 commits November 5, 2019 14:49

Formatting

2d28e84

version bump

21afcbe

bparrishMines requested a review from amirh as a code owner November 5, 2019 22:54

bparrishMines changed the title ~~[firebase_dynamic_links] support v1 embedding~~ [firebase_dynamic_links] support v2 embedding Nov 5, 2019

bparrishMines added 4 commits November 5, 2019 15:03

Update dynamic links version

4cb1fa9

Remove splashscreen from MainActivity

6925021

Add an actual test

48452fe

Merge branch 'master' of github.com:FirebaseExtended/flutterfire into…

0cf3010

… dynamic_embedding

amirh mentioned this pull request Nov 12, 2019

[firebase_dynamic_links] Support the v2 Android embedder plugins API flutter/flutter#41872

Closed

xster reviewed Nov 12, 2019

View reviewed changes

bparrishMines added 3 commits November 13, 2019 11:40

Documentation

526b88b

Merge branch 'master' of github.com:FirebaseExtended/flutterfire into…

8a0f6d5

… dynamic_embedding

Use activity paramter

5e6e755

bparrishMines merged commit 8bf9b22 into firebase:master Nov 21, 2019

bparrishMines deleted the dynamic_embedding branch November 21, 2019 19:05

firebase locked and limited conversation to collaborators Aug 8, 2020

[firebase_dynamic_links] support v2 embedding #1372

[firebase_dynamic_links] support v2 embedding #1372

Uh oh!

Conversation

bparrishMines commented Nov 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mklim Nov 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xster commented Nov 12, 2019

Uh oh!

xster commented Nov 21, 2019

Uh oh!

Uh oh!

bparrishMines commented Nov 5, 2019 •

edited

Loading

mklim Nov 13, 2019 •

edited

Loading