Skip to content

Conversation

amrfarid140
Copy link
Contributor

Description

Implementation for cloud_firestore_web plugin

Related Issues

This is part of the changes mentioned in this PR.

It is a follow-up of an the following merged [PR1] (#1686) & [PR2] (#1945)

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.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • If the pull request affects only one plugin, the PR title starts with the name of the plugin in brackets (e.g. [cloud_firestore])
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (flutter analyze) does not report any problems on my PR.
  • I read and followed the Flutter Style Guide.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy.
  • I updated CHANGELOG.md to add a description of the change.
  • I signed the CLA.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
  • No, this is not a breaking change.

- Migrated DocumentReference and CollectionReference to use interfaces
- added where support
- fixed test failure causes
- use any as versions
Created FieldValueFactory to provide platform-specific FieldValue instances
Added NO-OP folders for android & ios in firestore web package
- Removed unnecessary tracking of collection reference delegate and used the more generic query type instead
- fixed breakage
@ditman
Copy link
Contributor

ditman commented Feb 8, 2020

Taking a look at the failing checks, look like transient network failures. Relaunching.

@flowhorn
Copy link

flowhorn commented Feb 9, 2020

FirestoreWeb was renamed to CloudFirestoreWeb but this was not changed for the generated_plugin_registrant.dart file, which gets automatically created.


/// Web implementation for [FirestorePlatform]
/// delegates calls to firestore web plugin
class CloudFirestoreWeb extends FirestorePlatform {
Copy link
Contributor

@collinjackson collinjackson Feb 9, 2020

Choose a reason for hiding this comment

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

Despite the filename, I think this class should be FirestoreWeb rather than CloudFirestoreWeb.

The package was originally named firebase_firestore and I renamed it cloud_firestore in response to an internal request (see flutter/flutter#12388). My understanding from talking to the Firebase team is that firebase_firestore is now the preferred package name and that classes shouldn't be using a Cloud prefix. At some point we might want to rename this package back (updating to 1.0 might be the right time for that). I filed #1964 as a tracking issue, but for now we should try to be consistent with what the app-facing and platform interface packages are doing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. I've reverted the class name change.

- Revered FirestoreWeb rename
@collinjackson collinjackson self-requested a review February 10, 2020 13:21
Copy link
Contributor

@collinjackson collinjackson left a comment

Choose a reason for hiding this comment

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

image

@ditman
Copy link
Contributor

ditman commented Feb 10, 2020

Looking at the check that is red.

@ditman ditman merged commit 3ff0982 into firebase:master Feb 10, 2020
@ditman
Copy link
Contributor

ditman commented Feb 10, 2020

Cloud Firestore Web has been merged and published:

https://pub.dev/packages/cloud_firestore_web/versions/0.1.0

(After endorsing, we'll have to update the README with the latest info on how to set up the package, the current version is quite outdated by now)

@rodydavis
Copy link

Thank you for the hard work!!

@mvershinin-chwy
Copy link

Hey guys, thank you so much for great work. I tried to utilize it but getting these errors. Do you know a reason or suggestion how to fix it.
flutter build web

Target dart2js failed: Exception: lib/generated_plugin_registrant.dart:6:8:
Error: Error when reading '../../../flutter/.pub-cache/hosted/pub.dartlang.org/cloud_firestore_web-0.1.0/lib/firestore_web.dart': Error reading
'../../../flutter/.pub-cache/hosted/pub.dartlang.org/cloud_firestore_web-0.1.0/lib/firestore_web.dart'  (No such file or directory)
import 'package:cloud_firestore_web/firestore_web.dart';
       ^
lib/generated_plugin_registrant.dart:13:51:
Error: Getter not found: 'FirestoreWeb'.
  FirestoreWeb.registerWith(registry.registrarFor(FirestoreWeb));
                                                  ^^^^^^^^^^^^
lib/generated_plugin_registrant.dart:13:3:
Error: Getter not found: 'FirestoreWeb'.
  FirestoreWeb.registerWith(registry.registrarFor(FirestoreWeb));
  ^^^^^^^^^^^^
Error: Compilation failed.

plugin:
platforms:
web:
pluginClass: FirestoreWeb
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be FirestoreWebPlugin, I think!

@ditman
Copy link
Contributor

ditman commented Feb 10, 2020

@maximchewy thanks for reporting, I'll push a fix ASAP.

@amrfarid140
Copy link
Contributor Author

amrfarid140 commented Feb 10, 2020

Thanks @maximchewy.

Looks like I forgot to update fileName in pubspec.yaml when the file was renamed 😵

It should be:

flutter:
  plugin:
    platforms:
      web:
        pluginClass: FirestoreWeb
        fileName: cloud_firestore_web.dart

That should resolve the issue.

@ditman are you pushing the fix or do you want me to create a PR ?

@ditman
Copy link
Contributor

ditman commented Feb 10, 2020

@ditman are you pushing the fix or do you want me to create a PR ?

@amrfarid140 I can get the fix pushed, go get some sleep :P

@amrfarid140
Copy link
Contributor Author

amrfarid140 commented Feb 10, 2020 via email

@ditman
Copy link
Contributor

ditman commented Feb 10, 2020

@maximchewy we've published 0.1.0+1 with a fix for the issue you're seeing

@mvershinin-chwy
Copy link

@amrfarid140 @ditman thanks for quick response and fix. I was able to save data from flutter web to firestore. Great job guys

@ditman
Copy link
Contributor

ditman commented Feb 11, 2020

@maximchewy awesome!

@firebase firebase locked and limited conversation to collaborators Aug 3, 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.

7 participants