search

baumblatt / capacitor-firebase-auth

Capacitor Firebase Authentication Plugin
MIT License
261 stars 129 forks source link

Support Capacitor 3, Firebase 8, and Apple Sign-In on all platforms. #146

Closed lincolnthree closed 3 years ago

lincolnthree commented 3 years ago

Hey there,

So... first, I'd like to say thank you for your great work on this Capacitor Plugin. Why it's not a core feature of Capacitor/official, I have no idea, and.... I also want to explain this pull request and say a few things:

  1. I know it is a mess. It's not in a branch. And it's got way too many things to include all at once. Sorry about that.
  2. However, I was determined to get things working, and wasn't feeling like following any rules or 'contributor conventions'. Figured if you are interested, we could probably clean it up later. So consider this just a prototype where you can see all the changes I made in one place.
  3. I did not see the 2.4.0-alpha branch until it was too late. I'm sorry.
  4. At worst, I wanted to provide this as an example, and to make it easier for the maintainers to move ahead on a few of these:

Things I've done

So cheers, please take this as what it is, which is just a brain dump. I'm going to be using this fork in production until things catch up a bit in the main repo.

Let me know if this is at all helpful, or how you'd like to proceed.

One followup thought that I did not attempt because I already have firebase auth working on the web in my PWA, is... It's probably worth implementing an option to configure use of loginWithRedirect instead of loginWithPopup, because otherwise this won't work in PWAs (pop-ups are not supported.) I may take a stab at that and try to replace my current integration with the web implementation here.

lincolnthree commented 3 years ago

This also fixes: https://github.com/baumblatt/capacitor-firebase-auth/pull/141

mesqueeb commented 3 years ago

Is this fork also still usable with capacitor 2? Or does it need capacitor 3?

lincolnthree commented 3 years ago

Requires Capacitor 3.

baumblatt commented 3 years ago

Hello @lincolnthree ,

First of all, really thank you for the great job review several point of this plugin.

I wondering if you could change just two little things:

Next steps:

Looking forward to hearing from you.

Best regards, Bernardo Baumblatt.

lincolnthree commented 3 years ago

Hey @baumblatt, thanks for checking out the work I've put into this. It may take some time to go through this and re-do everything and clean it up. We are in the middle of a launch, so .. have quite a lot going on (hence my rush, and sloppy PR in the first place ;) .)

  • Use the next branch, there is a little work there that I don't want to loose.

Yes :)

  • Don't commit the dist folder, this goes on NPM package only.

Yes. I chose to do this so that we could install directly from GitHub to our build process without publishing to NPM. This should certainly be omitted when sending a real PR. (Again, we're in a rush and don't like re-publishing other people's NPM artifacts. It creates a bigger mess in the larger repo -- and even in our private repo.)

Next steps:

  • Review your PR [you]

Will do when I can! Not sure exactly when that will be, but will see if I can pull some time together :) There is still more work to be done with Capacitor 3 upgrade (to use ALL new/non-deprecated APIs, but this should work for now.)

  • Merge and Release it as alfa version (v3.0.0-alpha) [me]
  • Test your application and send me some evidences [you]
  • Release as lastest (v3.0.0) [me]

Sounds great. Let's make it happen.

mesqueeb commented 3 years ago

@lincolnthree @baumblatt is there anything I can help with?

lincolnthree commented 3 years ago

@mesqueeb @baumblatt Apologies - We launched our new app a few weeks ago and are still playing catch-up. I do not know when I'll be able to finish this.

baumblatt commented 3 years ago

@mesqueeb @baumblatt Apologies - We launched our new app a few weeks ago and are still playing catch-up. I do not know when I'll be able to finish this.

Hello @lincolnthree , we all know how those things are, it's all good, take your time.

Anyway, I willing to made the dirty work, the merge of everything, do you see any problem? If you agreed, I can clone your repository and put all your code on the next branch and publish an alfa version.

Looking forward to have your agreement in time to made it this weekend!

Best regards

lincolnthree commented 3 years ago

Please feel free, @baumblatt! I don't want to hold anything up. I may be around this weekend doing some work on our app as well so if you have any questions let me know. Github needs a messages app :)

mesqueeb commented 3 years ago

if there is any grunt work to be done, let me know how I can help!

mesqueeb commented 3 years ago

@lincolnthree @baumblatt there seems to be an indentation problem with the current version:

image

I feel like the next version you two are working on is so close, I won't bother open a PR! But try to include a fix for this if you can!!

baumblatt commented 3 years ago

Hello,

The job is almost finished, the stats:

I will close this and we will continue on issue #136

Best regards

lincolnthree commented 3 years ago

@baumblatt Awesome!!! Thanks so much for taking this to the finish line. Our release is not going super smoothly so I really appreciate it :) Looking forward to testing the new branch.