-
Notifications
You must be signed in to change notification settings - Fork 4k
feat(firebase_auth): implement support for useEmulator
#4263
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
Conversation
83e78bc
to
b125b30
Compare
Flaky test: android
apple
|
Please keep this PR in mind and approve it as soon as possible. This functionality is important, the sdks cannot be so outdated, they must have the same functionality as the native ones. Thank you. |
Thanks @alejandroaap for the vote of confidence To any reviewers, please note I understand this needs collaboration (and associated re-shaping as requested) to land, I'm ready to do that. In particular I consider this the last part of a series of PRs that should go in first, and as they go in, commits will disappear (via git rebase / re-push to keep it clean + easy to review ultimately) from here First up is testing to prove things do not break as change comes in: #4331 It will be a journey to get here in other words @alejandroaap - as each of those PRs may need to be reshaped a bit until accepted - please be patient |
b125b30
to
e3e4d7b
Compare
For those following along at home, we are hard at work in the new year We're almost there |
3051c60
to
422597e
Compare
Ready to go, I think. The PR description contents have been updated to reflect all relevant information and instructions for users to use the feature in testing. |
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.
Good work, @mikehardy 💪 . I've made a few points that might warrant further attention.
I ran integration tests locally, and the setup was working for me, albeit with a number of tests failures. Some errors are of the same type, so once debugged, it will probably solve a few test failures. Good start though 😄
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.
I replied but I have to explicitly comment as well on mobile github...
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.
In light of feedback, LGTM 👍
422597e
to
eb48826
Compare
For those following along - thinking of testing this: This has had a couple rounds of review now and I've just pushed what should be the final version. There are a pile of firebase_auth PRs in progress right this moment, and this is not first in the queue. So it will sit for a little bit while those land, then I'll ingest the changes here and this should land as is. Nothing is permanent in software but I don't anticipate anything will change, so it should be good to work with if you want to follow the testing instructions above and use it in your project Cheers |
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.
It's shipping time! Thank you for sorting this @mikehardy
LGTM
useEmulator
Available in |
Fantastic! Thanks for all the help everyone |
Thanks for this new feature! |
Alternatively - |
Co-authored-by: Mike Hardy <[email protected]> Co-authored-by: Mike Diarmid <[email protected]>
Co-authored-by: Mike Hardy <[email protected]> Co-authored-by: Mike Diarmid <[email protected]>
Web: iOS: both are working fine. but, android, i getting error. PF i tried above mentioned two methods. Error: Physical device is working fine. with FirebaseAuth.instance.useEmulator('http://10.1.80.63:9099'); |
on the android emulator you should just use localhost, it will transparently remap to 10.0.2.2 and in my testing while implementing this it was working assuming you had the appropriate network security policy built into the app that allowed insecure local transport (a change in AndroidManifest.xml that is visible in the PR) |
I wanna use this on stable version of flutter is this possible? |
I too wonder if there are plans to make this available on the stable. If not, can we get advice for what needs to be done do get access to this? Which flutter channel should we be on, and what changes to the pubspec do we need to make? |
If you switch to master, then the instructions at the top will work for you |
Thanks for the tip in trying the no-nullsafety branch. I did try to switch over to that branch, but if I see it correct, there seems to be some reliance on the null-branch anyway. Here is what I added to pubspec, taken from the above
And what I get when I run 'flutter pub get'
That is a strange message I think since it somehow seems to refer to to a nullsafety version, but I have to say, I don't see where it does that. I'm not so well-versed in pub and dart to really understand what is going on here, so posting this message in the hope someone might be able to explain it. For full reference, if I switch out the 'no-nullsafety' to 'master' then I don't get the error message that the package doesn't exist, but rather the error that versions are incompatible. Also, here is the rest of my dependencies:
|
on firebase_auth 0.20.0+1 but still don't see the useEmulator method? |
Let's follow the trail:
There are no tags on that commit yet. So I would assume it has not been released. Looking at the list of commits on the branch, it seems that releases will be tagged and see a commit https://github.com/FirebaseExtended/flutterfire/commits/no-nullsafety So I'm 99% sure now it just hasn't been released Patience, all :-) |
The following package versions have been published with this in for the non-nullsafety versions:
|
@Salakar, the hero the world needs right now ;-) |
I tried it on web but it crashes. Works fine on iOS and Android.
|
Awesome! |
@pxsanghyo oh no! It's likely this is a bug in the underlying implementation yes, I was able to test android/iOS pretty thoroughly but not web. So, thank you for your report and sorry it gave trouble. Thanks a bunch for the version + stack trace, that helps. Your version corresponds to the no-nullsafety branch: https://github.com/FirebaseExtended/flutterfire/commits/no-nullsafety/packages/firebase_auth And it appears we are on this line: https://github.com/FirebaseExtended/flutterfire/blob/2eb0751269b679ced0c033823d896aa4e8daea82/packages/firebase_auth/firebase_auth_web/lib/firebase_auth_web.dart#L313 But that does not seem correct, it seems that we should be here? https://github.com/FirebaseExtended/flutterfire/blob/2eb0751269b679ced0c033823d896aa4e8daea82/packages/firebase_auth/firebase_auth_web/lib/firebase_auth_web.dart#L343 ...so I'm not sure I understand the error but I still lean strongly towards "you are the first to use it and report back, and there is probably some trivial implementation error on my part" so I'd like to know more about why the line numbers don't meet expectations. If it is not an implementation error I separately suspect this may have to do with the firebase-js-sdk versions not being compatible (because we did not mention anywhere what was needed!) - It appears you need to set them yourself in your HTML: Further it appears that the firebase 8.2.4 version may be the first one that works for useEmulator - can you either confirm you are already using this version (or higher), or if not then upgrade to it and test with it? Note that for android, localhost will be remapped for you to 10.0.2.2 as a developer experience enhancement, so you may remove some of your code I think: |
@markhardy awesome, thanks for looking into it. The firebase-js-sdk version is/was set at <script src="https://www.gstatic.com/firebasejs/8.2.7/firebase-app.js"></script>
<script src="https://www.gstatic.com/firebasejs/8.2.7/firebase-analytics.js"></script>
<script src="https://www.gstatic.com/firebasejs/8.2.7/firebase-auth.js"></script>
<script src="https://www.gstatic.com/firebasejs/8.2.7/firebase-firestore.js"></script>
<script>
var firebaseConfig = {};
firebase.initializeApp(firebaseConfig);
firebase.analytics();
</script> Thanks for the pointer about localhost being remapped to 10.0.2.2. Let me know if you need me to try anything else. |
@pxsanghyo you definitely want me versus Mark Hardy, though I am certain he is a nice person :-) |
@mikehardy lol sorry about that. I created issue #5096. Thanks |
Description
implements
FirebaseAuth.useEmulator(string origin)
Review notes:
Future work:
I implemented this for react-native-firebase but similar to caution in my forward-port PR here, I am new to Dart/Flutter/FlutterFire so I need assistance with some of the fundamentals. Be gentle :-)
Please help testing the PR
Add the following to your pubspec.yaml:
User notes:
Related Issues
#3969
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. Updating the
pubspec.yaml
and changelogs is not required.///
).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?