[cloud_firestore] Adds equality comparison to FieldValue (part 1, platform interface)

[collinjackson]

Description

Adds equality comparison to FieldValue. This is useful for testing.

This pull request will be landed in 3 parts -- first we need to update the platform interface, then the web version, then the app-facing package.

This is part one of a 3-part series that addresses #2018

Part two is here: #2062
Part three is here: #2063

[mehmetf]

Can this be a generic instead of dynamic?

[collinjackson]

That's a good suggestion, and I think it won't be a breaking change. Thanks.

[collinjackson]

Alas, I can't figure out how to switch to a generic version without some sort of deprecated leftovers. Federated plugins are supposed to strongly avoid breaking changes in the platform interface even if it makes the interface a bit uglier.

I think our options are:

1. Leave in a deprecated compatibility shim class to support old platform implementations (edit: code sample is below).
2. Land a breaking change with a major version bump and make the version solver sad.
3. Land a breaking change without a version bump and hope not too many people experience compatibility issues.
4. Forgo the generic version for now, and document the reason in a comment.

The last option seems the least painful - it's still pretty readable without a generic type argument. And since only platform implementers have to look at it, not very many people are impacted if this class is a bit harder to understand.

@amirh if you have any feedback lmk

[amirh]

f2f discussion with Collin:

Meta discussion - a complex platform interface like this one proves to be much harder to maintain with no breaking changes than we've expected (even that we were willing to pay in API dirtyness). We should re-evaluate our guidelines for platform interfaces, possibly by finding ways to allow breaking changes (though this requirement seems core to federation), possibly by enforcing simplicity of platform interface. As this is the first time we encounter such a complex scenario it's probably useful to wait a bit for something like this to happen again so we have more date to evaluate against.

As for a short term solution for this discussion - I support going with dynamic right now, especially as it is forward compatible with the alternative approach of deprecating the entire class (in which case we could introduce a new class with generics), or if we find a way to make breaking changes we could change to generics later.

[ditman]

You guys know I'd lean towards option 2.

(But that can cause users to be stuck in unmaintained versions of the plugin for too long)

[collinjackson]

Here is what option 1 would look like. It's actually too ugly. The argument for not switching to it now is that we could always switch to this later, and it leaves our options open to do something else. The argument for doing it now is that it's technical debt and we should be fixing mistakes while the plugin is still young and has few platform implementers.

But we may decide that having multiple platform interface classes in a plugin and using language features like templating a bad idea, and instead try to put as much functionality as possible into one platform interface class (like how MethodChannel works). I'd like to move forward with option 4 for now to unblock the customer, and re-evaluate once we've done a few more multi-class plugins.

import 'package:plugin_platform_interface/plugin_platform_interface.dart';

import 'package:cloud_firestore_platform_interface/src/method_channel/method_channel_field_value_factory.dart';
import 'package:cloud_firestore_platform_interface/cloud_firestore_platform_interface.dart';

/// Class kept in place only to avoid a breaking change.
@Deprecated('Use FieldValueFactoryPlatformBase instead.')
abstract class FieldValuePlatform extends FieldValueFactoryPlatformBase<dynamic> {
  /// Compatibility method that sets the instance of [FieldValueFactoryPlatformBase].
  static set instance(FieldValueFactoryPlatform instance) {
    FieldValueFactoryPlatformBase.instance = instance;
  }
}

/// An interface for a factory that is used to build [FieldValuePlatform] according to
/// Platform (web or mobile)
///
/// This class would make sense as a generic, but is not doing so to avoid a breaking change.
abstract class FieldValueFactoryPlatformBase<T> extends PlatformInterface {
  /// Constructor to initialize the PlatformInterface base class
  FieldValueFactoryPlatformBase() : super(token: _token);

  /// Current instance of [FieldValueFactoryPlatform]
  static FieldValueFactoryPlatform get instance => _instance;

  static FieldValueFactoryPlatform _instance = MethodChannelFieldValueFactory();

  /// Sets the default instance of [FieldValueFactoryPlatform] which is used to build
  /// [FieldValuePlatform] items
  static set instance(FieldValueFactoryPlatform instance) {
    PlatformInterface.verifyToken(instance, _token);
    _instance = instance;
  }

  static final Object _token = Object();

  /// Throws an [AssertionError] if [instance] does not extend
  /// [FieldValueFactoryPlatform].
  ///
  /// This is used by the app-facing [FieldValueFactory] to ensure that
  /// the object in which it's going to delegate calls has been
  /// constructed properly.
  static verifyExtends(FieldValueFactoryPlatform instance) {
    PlatformInterface.verifyToken(instance, _token);
  }

  /// Returns a special value that tells the server to union the given elements
  /// with any array value that already exists on the server.
  ///
  /// Each specified element that doesn't already exist in the array will be
  /// added to the end. If the field being modified is not already an array it
  /// will be overwritten with an array containing exactly the specified
  /// elements.
  T arrayUnion(List<dynamic> elements) {
    throw UnimplementedError("arrayUnion() is not implemented");
  }

  /// Returns a special value that tells the server to remove the given
  /// elements from any array value that already exists on the server.
  ///
  /// All instances of each element specified will be removed from the array.
  /// If the field being modified is not already an array it will be overwritten
  /// with an empty array.
  T arrayRemove(List<dynamic> elements) {
    throw UnimplementedError("arrayRemove() is not implemented");
  }

  /// Returns a sentinel for use with update() to mark a field for deletion.
  T delete() {
    throw UnimplementedError("delete() is not implemented");
  }

  /// Returns a sentinel for use with set() or update() to include a
  /// server-generated timestamp in the written data.
  T serverTimestamp() {
    throw UnimplementedError("serverTimestamp() is not implemented");
  }

  /// Returns a special value for use with set() or update() that tells the
  /// server to increment the field’s current value by the given value.
  T increment(num value) {
    throw UnimplementedError("increment() is not implemented");
  }
}

[amirh]

I don't rule out option 2, in fact I wish we'd find a solid way to enabled it. However I feel like we need to have answers to problems that arise from having different platform implementations depend on different platform interface major versions. And it will probably take us at least a few days to brainstorm and evaluate approaches.

I'm ok with the other options as the immediate fix, as if we come up with the proper way to allow breaking changes to platform interfaces we could just clean that up with a breaking change to the PI.

[collinjackson]

Moving forward with option 4 for now since it gives us the freedom to pursue any of the other options in the future.

[ditman]

I think we need a place (maybe a github issue?) to centralize all these little things where we're borrowing technical debt for expediency's sake, so we don't forget about coming later and doing a big breaking change with a bunch of changes that we like better.

[collinjackson]

I think we need a place (maybe a github issue?) to centralize all these little things where we're borrowing technical debt for expediency's sake, so we don't forget about coming later and doing a big breaking change with a bunch of changes that we like better.

I opened an issue in the FlutterFire issue tracker, but feel free to move to flutter/plugins if you prefer.