Revert Firestore data() to data

[feinstein]

I recently updated my Firestore package to the latest version and the old data getter became a function data(). This is really strange to use as we need to call it as myDoc.data()['note'], instead of just d.data['note'] which is much more readable and makes more sense. I read the source-code and really can't see why the change was made.

Also I had several problems updating as not only it isn't backwards compatible, but also in dynamic code it will compile and just break at runtime saying that a function can't be a Map or something like this, I don't remember exactly.

Maybe this looks like something small to many, but I really don't like this as it looks like the API became more cumbersome and less polished.

[Ehesp]

There is a reasoning for this change - it should be possible to call data() with SnapshotOptions however I can't yet figure out how to do this with a native bridge. I thought that once that's figured out it'll then be a feature change rather than a breaking change, since it'd be an optional argument.

[feinstein]

I am sorry but I don't quite agree with this change then. You are breaking the API so maybe later in the future it will have some purpose. Wouldn't it make more sense to only break it once it has an actual purpose? Until then keep it as is? Sounds a bit as premature optimization to me to be honest.

Also, can't SnapshotOptions be passed to get() instead? I am not sure as I didn't actually understood the usage of SnapshotOptions.

[Ehesp]

Yeah since so much else was going to change the plan was to introduce breaking changes on an agreed api, and then prevent breaking changes as we move forwards.

Maybe it could yeah, however we'd still have to reverse a change which would then again be breaking 😢

[feinstein]

I understand your reasoning, IMHO I would revert this, as soon as possible because I suppose the new API was introduced quite recently so the impact is still small.

There's also room for another idea: There could be a data getter and a dataWithOption() function (or probably another better name 😅). Since most of us never complained or even missed the SnapshotOptions, I bet most people will just use data and on rare cases would turn to dataWithOption(). This way the API is simpler for the vast majority of use cases, and the more complex API is just used as needed.

[Salakar]

Wouldn't doing myDoc.get('note') be the safer/better/cleaner option, especially if you're only getting single values like that. .data() is really aimed at if you need all the data as a raw Map.

Here's some additional points on the two;

• .get() supports getting deeply nested values using a string or a FieldPath instance, e.g. .get('note.author')
• .get() with throw a StateError if a value does not exist, halting further code execution and potential nullability issues.
• .data()['note'] will return null if it doesn't exist or if the value is actually null on your Firestore database, so making the assumption of it's null so it doesn't exist would be incorrect, it could exist but the value itself is stored as null

[feinstein]

Honestly I didn't even see get on the docs, but looking now I can see one reference, so I agree it should be more emphasized.

I don't use it because I integrate my queries with built_value so I usually do something like:

  Stream<List<Book>> getBooks(String libraryId) {
    return _firestore
        .collection('libraries')
        .doc(libraryId)
        .collection('books')
        .snapshots()
        .map((qs) {
      return qs.docs.map((d) {
        return serializers.deserializeWith(
          Book.serializer,
          d.data(),
        );
      }).toList();
    });
  }

Where Book is a Value Object generated by built_value with a Serializer.

But instead of having get, couldn't we just have an indexer using operator []? This way we could just have myDoc['note'] which seems a lot cleaner to me.

[Salakar]

But instead of having get, couldn't we just have an indexer using operator []? This way we could just have myDoc['note'] which seems a lot cleaner to me.

Do you have an example of what the implementation would look like? Thanks

[Ehesp]

Something like snapshot['foo.bar'] which forwards onto snapshot.get('foo.bar') could also work.

[feinstein]

I was going to give an example here, but it was so simple that I sent it as a PR with tests: #3387

It's basically:

dynamic operator [](dynamic field) => get(field);

[feinstein]

We could even improve the typing on get to support something like:

var enabled = myDoc.get<bool>('enabled');

Would love a PR for this 👀

I tried to bundle something together real quick for a test, but since the data comes as a Map<String, dynamic> and we can't change the dynamic nature of the data, I hit a dead end.

To be more specific, I get exceptions when execution hits this line:

https://github.com/FirebaseExtended/flutterfire/blob/7eb9e21f82a09c5cecebfc231b256545cf1ce073/packages/cloud_firestore/cloud_firestore_platform_interface/lib/src/platform_interface/platform_interface_document_snapshot.dart#L101

I extracts dynamic data from a Map, but if it doesn't match the given type <bool> it throws an exception at runtime.

IMO the whole purpose of a change in the typing system is to be able to catch type misamtches at compile time, but I don't know if this is even possible. In the end we will just be exchanging an exception in the client code for an exception inside the library itself.

Thoughts?