-
Notifications
You must be signed in to change notification settings - Fork 4k
[cloud_firestore] Add macOS support #1706
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[cloud_firestore] Add macOS support #1706
Conversation
78192b3
to
95a9b4d
Compare
There's a big refactor of this package in here to add web support, where the core plugin is being federated (there's a new platform interface and a whatever changes were needed to the core). Do you mind trying to integrate the PR above with your code and see if it's still working as expected? Thanks! |
I am aware of that PR but I don't expect it to break anything as this is just a copy of the ios implementation. I am happy to rebase and copy all changes once that PR is merged. |
@cbenhagen cool! I'm not setup to do macos PRs (can't run tests yet), but you tagged the right people here! |
@cbenhagen Yes! Thank you for working on this. |
I've attempted to add this to the example in the Firebase for Flutter codelab by adding this to the pubspec file:
It now seems to correctly grab the required pods when building for mac, but at runtime I'm getting this exception:
I've added the same Any idea what the issue may be? Does running firestore on mac require any other changes, perhaps platform specific dart code? |
@phest You would also need to add overrides for |
26d2f4d
to
a4cf81b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Someone with experience in Obj-C (@collinjackson? (ah, I see @franciscojma86 now!)) should review this! Very cool!
PS: would it make sense to split this into a cloud_firestore_darwin
package that contains the ios
and macos
platforms, instead of adding more stuff to the "core" plugin?
packages/cloud_firestore/cloud_firestore/darwin/Classes/UserAgent.h
Outdated
Show resolved
Hide resolved
packages/cloud_firestore/cloud_firestore/macos/Classes/FLTCloudFirestorePlugin.m
Show resolved
Hide resolved
packages/cloud_firestore/cloud_firestore/darwin/Classes/FLTCloudFirestorePlugin.m
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM modulo @ditman comments
@cbenhagen thank you. Did you mean like so ☝️? I'm seeing the compiler error below. Tried re-creating the XCode project from scratch but no luck.
|
@phest I removed that header file. Please update and retry. |
@phest you don't have to try. It doesn't work currently. If you need a solution today, copy the files in darwin to the macos directory for both plugins instead of symlinking. @collinjackson / @franciscojma86 the compiler does not like when I remove the header file from the darwin directory. It seems to look in that dir for the header instead of where the .m is linked to. Ideas? |
packages/cloud_firestore/cloud_firestore/darwin/Classes/UserAgent.h
Outdated
Show resolved
Hide resolved
I am investigating. (edit: waiting to see if the tests pass) |
packages/cloud_firestore/cloud_firestore/macos/cloud_firestore.podspec
Outdated
Show resolved
Hide resolved
@cbenhagen I just noticed that you also need to add a macos directory to the example app to run the macos integration tests. I'm going to try and add that myself and send you a patch. Sorry about that |
@franciscojma86 you can just commit to my branch as well. |
(Restarted all failing checks) |
@ditman please restart again. Looks like there was a network issue. |
Yep, they all look like network connectivity issues, restarted. |
Looks like all checks have passed @ditman . Thanks for working on this @cbenhagen ! |
@franciscojma86 this one should be ready to merge and release as well :) |
Sorry just saw this one :). I resolved the conflicts, once tests pass again I'll land it. Thanks! |
Description
Add macOS support.
Related Issues
#1653
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?