[cloud_firestore, cloud_firestore_web] Fix encoding issues in DocumentSnapshotPlatform and FieldValueFactoryWeb #2105
Conversation
``` I/flutter ( 4164): 00:07 +9: Firestore pagination with DocumentReference (#2044) [E] I/flutter ( 4164): Invalid argument: Instance of 'DocumentReference' I/flutter ( 4164): package:flutter/src/services/message_codecs.dart 392:7 StandardMessageCodec.writeValue I/flutter ( 4164): package:cloud_firestore_platform_interface/src/method_channel/utils/firestore_message_codec.dart 83:13 FirestoreMessageCodec.writeValue I/flutter ( 4164): package:flutter/src/services/message_codecs.dart 389:9 StandardMessageCodec.writeValue.<fn> I/flutter ( 4164): dart:collection-patch/compact_hash.dart 379:8 _LinkedHashMapMixin.forEach I/flutter ( 4164): package:flutter/src/services/message_codecs.dart 387:13 StandardMessageCodec.writeValue I/flutter ( 4164): package:cloud_firestore_platform_interface/src/method_channel/utils/firestore_message_codec.dart 83:13 FirestoreMessageCodec.writeValue ```
This is required for the arrayRemove/arrayUnion FieldValues.
Also tweak some integ tests so everything passes now, in Android.
packages/cloud_firestore/cloud_firestore/lib/src/utils/platform_utils.dart
Show resolved
Hide resolved
ditman
changed the title
|
This doesn't seem like a flake, but I don't know how to fix this :/ (the error seems to come from Firebase core?) |
|
That mac failure seems to be affecting all builds right now. @franciscojma86 any ideas? |
|
I just ran it locally and I'm getting |
|
@franciscojma86 #2099 is failing with the same error. Is this one of those cases where we need to merge in red to get the tree green again? |
|
Not quite sure. I seems like @collinjackson still need to update the PR with Jenn's comments. |
|
I'll try to get to #2099 today. Thanks for your patience. |
|
@collinjackson @franciscojma86 is it OK if I merge this with that check failing? Several other (seemingly unrelated) changes have been landed with that test in red. |
|
Yes, go ahead and land it, the tests are red on tip of tree so fixing so landing #2099 isn't going to improve the situation, it'll just unblock @franciscojma86 troubleshooting the failing mac test. I think that what may have happened is that 7a312ce passed the tests but then broke the build shortly afterwards when it was published. Having a way of testing the effect of firebase_core changes on other plugins in this repo prior to publication would be helpful here. We could roll back that PR to do so would effectively mean not having macos support and might do more harm than good. |
|
@franciscojma86 in the meantime you can try a clean checkout and build for mac and it should probably get you past the UserAgent.h issue. |
@collinjackson I think we should try to fix ASAP and roll forward, if possible (but that's a discussion for another ticket PR/Issue) |
|
I'm landing this with the |
|
I'll wait for the post-submit to finish before publishing this two plugins. |
|
The post-submit is not worse than the previous one, so I'm publishing this two plugins now. |
Description
This change reproduces the issue reported in #2044 with an Integ test, and fixes it.
While running Integ tests, I discovered some extra changes required to get them all to pass (on Android, at least), and shaved that Yak.
This also fixes another encoding issue in _web that was recently introduced. Web still has one broken Integ test (re: encoding of Timestamp objects), that I'll attempt to fix next.
/cc @amrfarid140
Related Issues
Closes #2044.
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]). This will ensure a smooth and quick review process.///).flutter analyze) does not report any problems on my PR.Breaking Change
Does your PR require plugin users to manually update their apps to accommodate your change?