Skip to content
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

Merge plugin #47

Closed
hugoblanc opened this issue Jul 17, 2019 · 11 comments
Closed

Merge plugin #47

hugoblanc opened this issue Jul 17, 2019 · 11 comments

Comments

@hugoblanc
Copy link

Feature request

Title

Merge plugin

Description

Ionic users are all using the good old cordova-plugin-firebase Ionic Native firebase
Which is not a good solution as described here: Issue 3038
Your repository and the wizpanda's repository are trying to achieve the same thing.

solution

Merge both repository would be ideal.
Then we should be able to create an ionic native plugin which does not tearing up the community.

reasons

This new repo could get more visibility with the ionic documentation, and bring clarity between all cordova-plugin-firebase forks

Describe alternatives you've considered

Not merge, and create an ionic native "plugin" with only one repository. This solutions leads to dupplication of solutions and the end of the unchoosen plugin, which is sad as you both seems invested into this plugin.

Additional context

I also posted on wizpanda repo feature 45

@dpa99c
Copy link
Owner

dpa99c commented Jul 17, 2019

The cordova-plugin-firebase-lib fork is not trying to achieve the same as this fork:
in v5.0.0 I have completely rewritten the native push notifications code for both Android & iOS to fix existing bugs and add new features.
In contrast, the Wizpanda fork currently just resolves build issues which currently exist in cordova-plugin-firebase but barely touches the native code.

For my own purposes, I will probably rewrite other native sections of the plugin to fix bugs and add functionality.

Before creating this fork, I asked the maintainers of cordova-plugin-firebase if they'd grant me maintainer status so I could directly publish my changes under cordova-plugin-firebase. However I did not get a response. Making PRs as a contributor to cordova-plugin-firebase also got me nowhere as the PRs were never merged.

Hence I decide to create this fork for my own use. You are also welcome to use it (or not).

If you wish this functionality to be merged into cordova-plugin-firebase or cordova-plugin-firebase-lib, you should raise the issue with the maintainers of those repositories.

@dpa99c dpa99c closed this as completed Jul 17, 2019
@sagrawal31
Copy link
Collaborator

Hi Dave, you are doing great and you have come so far in terms of keeping this repository up to date with latest build & changes.

Well, our intention in the start was to just fix the build issue because of breaking change release by Google Firebase but on the next immediate day, we decided to maintain the repository for existing & future users by improving the code regularly. And since 17th Jun 2019, we have done the following:

  1. Fix Android Firebase breaking change with backward compatibility (for cordova-android v6 & v7) without the use of cordova-plugin-androidx plugin.
  2. Improvement in log error functions in Crashlytics.
  3. Fix Android Firebase breaking change for cordova-android v8.
  4. Using the latest JS syntax.
  5. Formatted the entire code to and maintained a coding standard.
  6. Improving the docs & APIs.
  7. Migrating to Cocoapod with latest cordova-ios v9 without the use of cordova-plugin-cocoapod-support plugin.
  8. Removing or cleaning up the old useless code.
  9. Updating various dependencies to latest for performance improvement & bug fixes.
  10. Most important, started maintaining it before you and without the option to donate.

With due respect, well, this definitely not touched the native code majorly but that doesn't mean we are just resolving build issues which currently exist in the original plugin. It's just a matter of time & availability.

We both (at least us) have started maintaining the fork to give back to the open-source community so that the developers across the globe can benefit from it and can keep their focus on their core business logic. And our intention is still the same.

Request you to please respect the work we tried to deliver so far. Your words were more like "they did nothing to maintain".

Now, coming to the main point, if developers are getting confused or diverted to two different repositories, we will be happy to achieve our repository and send them to this repository. But I have a concern. You recently changed the names of some JS API which will not work with the Ionic native wrapper so the Ionic developers might not be able to use it. So are you going to update the same in the Ionic wrapper? And even if you update those Ionic TS wrappers, Ionic 3 users will not be able to use that.

Please let us know your thoughts!

@dpa99c
Copy link
Owner

dpa99c commented Jul 19, 2019

@sagrawal31 I apologise if my comment offended you - that was not my intention.
I was merely invalidating @hugoblanc's assertion that "Your repository and the wizpanda's repository are trying to achieve the same thing" because this is not the case:
I appreciate the effort that you have made in resolving existing issues and tech debt with cordova-plugin-firebase however my focus, in addition to that, has been reworking the plugin functionality to rationalise the behaviour across platforms and iron out quirks that have niggled me since I first made use of the original cordova-plugin-firebase.

Since I made the leap and decided to make this fork (rather than feeding PRs back to cordova-plugin-firebase which were merged too slowly) my focus has initially been on the cloud messaging functionality, since this is what I make most use of in my own apps, but I will probably move on to rationalise other areas of functionality.
As such, my plugin has diverged (and will continue to) from cordova-plugin-firebase in that I have made (and will make further) breaking changes to the API and behaviour of the plugin in order to rationalise it for my own needs. This may involve adding new functionality or changing the behaviour/interface of existing functionality.

So with regard to Ionic Native, you are correct that this plugin will no longer work with the cordova-plugin-firebase wrapper but I would argue that's acceptable since this plugin is no longer cordova-plugin-firebase. If Ionic users want to use this fork, they can do so by directly calling the JS API (without the Ionic wrapper) or if someone wants to create a PR for Ionic Native to add an explicit wrapper for this fork, they are more than welcome.

So I emphasise again, this fork is primarily for my own purposes (to be used in my own production apps) however I'm happy to take on board feedback for other users of it since it's in my interests that other devs use my code as it increases the likelihood of bugs/issues being spotted and fixed.

Ideally I would not have to create/maintain this fork if the maintainers of cordova-plugin-firebase were more active - then I could have committed these changes directly back to the original repo - as I have enough open-source projects to maintain as it is.

If I were creating a Cordova plugin wrapper for the Firebase SDK from scratch, I would have taken the componentised approach of @chemerisuk and created a separate plugin for each logical component of the Firebase SDK rather than this monolithic approach which includes components you may not need in your app build.
Once I have stabilised the plugin such that it resolves all the issues/implements all the functionality I personally require, I will try to collaborate with @chemerisuk to feed that functionality back to his plugins so I can shut down this fork entirely.

Hope that explains my rationale and once again I apologise if you were offended as that was not my intention.

@sagrawal31
Copy link
Collaborator

I really appreciate your apology and in-depth reply, Dave. And I apologies too for taking your comment wrong and for this delay in response. 😄

So with regard to Ionic Native, you are correct that this plugin will no longer work with the cordova-plugin-firebase wrapper but I would argue that's acceptable since this plugin is no longer cordova-plugin-firebase. If Ionic users want to use this fork, they can do so by directly calling the JS API (without the Ionic wrapper) or if someone wants to create a PR for Ionic Native to add an explicit wrapper for this fork, they are more than welcome.

Agreed. We can do that in the future but developers are more tend to use the native wrappers (because they feel safe using it).

If I were creating a Cordova plugin wrapper for the Firebase SDK from scratch, I would have taken the componentised approach of @chemerisuk and created a separate plugin for each logical component of the Firebase SDK rather than this monolithic approach which includes components you may not need in your app build.

Totally agreed. The original fork (and hence we are) is adding many firebase features which are of no use to everybody.

So looks like we are now on the same page. So here are my questions & thoughts:

  1. I'll put banners & headings on our fork to divert developers to this repository
  2. I'll achieve our repository and will stop adding new commits to it
  3. Since developers are coming and others will be coming to your repo, are you ready to take entire traffic to you? (this means, more issues & PRs)
  4. You can add me as a contributor to your repository and I can do some cleanup and fixes too (by raising PRs)
  5. What needs to be done for existing API users (since APIs in your fork has changed)

@dpa99c @hugoblanc your thoughts, please.

@dpa99c
Copy link
Owner

dpa99c commented Jul 22, 2019

@sagrawal31 Shashank, I'm glad we've sorted out that misunderstanding and I'd be happy to collaborate with you on this.

I've added you to this repo as a contributor and added a credits section to the README in anticipation of your contributions 😁 - looking at the commit history in cordova-plugin-firebase-lib, there's some useful stuff in your repository that would be good to bring across to this one.

What needs to be done for existing API users (since APIs in your fork has changed)

That's a good idea - I'll create an issue to capture this: maybe we should add a sub-section under Migrating from cordova-plugin-firebase in the README which explicitly states the breaking API changes. It should probably also make it clear that ionic-native/@firebase will not work with this plugin due to the breaking API changes.

@dpa99c
Copy link
Owner

dpa99c commented Jul 22, 2019

I'll create an issue to capture this

#51

@sagrawal31
Copy link
Collaborator

@sagrawal31 Shashank, I'm glad we've sorted out that misunderstanding and I'd be happy to collaborate with you on this.

Nice 😄

I've added you to this repo as a contributor and added a credits section to the README in anticipation of your contributions 😁 - looking at the commit history in cordova-plugin-firebase-lib, there's some useful stuff in your repository that would be good to bring across to this one.

Thank you so much for this. Really appreciate.

I'll start pushing some updates to the repo and docs.

@hugoblanc
Copy link
Author

On my side, I will work on the ionic native wrapper
Thanks a lot for your effort !

@patryk-fuhrman
Copy link

On my side, I will work on the ionic native wrapper
Thanks a lot for your effort !

Pull request

@l3ender
Copy link

l3ender commented Jul 25, 2019

Thanks for everyone working hard to standardize and make a single place for Firebase. I think it will really help the community.

Are there any particular steps to follow when migrating from wizpanda/cordova-plugin-firebase-lib to this plugin? I am using 5.1.1 of that plugin.

Thanks!

@hugoblanc
Copy link
Author

Thanks for everyone working hard to standardize and make a single place for Firebase. I think it will really help the community.

Are there any particular steps to follow when migrating from wizpanda/cordova-plugin-firebase-lib to this plugin? I am using 5.1.1 of that plugin.

Thanks!

cordova-plugin-firebase-lib was quite equivalent to the original cordova-plugin-firebase so you could follow Migration guide. As said dpa99c: if you're ionic user: native wrapper will not work with this new plugin, but a pull request is coming

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants