search

ReallySmallSoftware / cordova-plugin-firestore

A Google Firebase Firestore plugin to enable realtime synchronisation between app and cloud and automatically handle limited connectivity.
Other
22 stars 10 forks source link

Multiple new features and refactoring #17

Closed jggc closed 5 years ago

jggc commented 5 years ago

Proposal

@ReallySmallSoftware I am opening this pull request now to start a discussion on merging my multiple changes.

My situation is : we are now using my fork of this plugin in production as a drop-in replacement for the firebase-js-sdk. That means that we will need to commit without delay new features or fixes as we may need them in the future. This fork has a few small breaking changes from the previous version that I will update here tomorrow.

I think my new code gives a lot of potential to this project to be used easily in any cordova application that started out with firebase-js-sdk or that needs transparent fully featured firebase-js-sdk browser support. I am also improving javascript code quality by adding some tests.

Let me know if you are interested in merging this (once I've completed the SubCollection/SubDocument cleanup and rebase) and adding me as a collaborator or if you'd rather have us staying on my fork.

ReallySmallSoftware commented 5 years ago

Thank you, I will take a look. I did commit some changes and release a new version a couple of weeks ago which covers some of this functionality, so I'll be interested to compare what we have both done.

ReallySmallSoftware commented 5 years ago

@jggc I've taken a very quick look at this - I won't be able to do anything properly until the weekend I expect. However, on the face of it the changes you have made are similar to the changes I made on my latest release, which is pleasing. As far as I can see, we both basically removed the sub collection stuff and just re-used the existing collection and also the same for the sub document.

I have deleted quite a lot of native code in my latest release - removing the iOS and Android code that corresponds to the sub doc/collection JS. If we are on the same track then this might be of interest to you.

The addition of the tests is great as are the other two bits of functionality.

Whilst I feel the renaming adds noise amongst all the other changes it is what it is.

Due to our slightly conflicting changes I think this will need to be merged manually, but obviously having your tests mitigates a lot of the risk.

So I would like to merge your changes, and I see little point in having a separate fork once this merge is complete as it would be a shame to duplicate effort.

I'll keep an eye for when you believe you have finished your changes and if you are able to pull in any of the work I have done to your fork that will obviously make the merge process easier.

jggc commented 5 years ago

All right, thank you for having a look a it so quickly.

I had a look on how you made the subcollection frontend implementation and there are a few little things we did differently but yeah mostly the idea was to use data recursion between documentReference and collectionReference as I mentionned when I opened #16 .

The main difference is that I copied the way firebase-js-sdk uses a Path object to manage .path and .id. Instead you made a utility function which I guess is fine too.

I will work on making a clean and mergeable pull-request, I just wanted you to confirm that I wasn't wasting my time before completing my work.

jggc commented 5 years ago

I just pushed what should be close to a mergeable 3.0.0 version. See the Changelog entry in the last commit for details.

I made it a major version since there is a breaking change in QuerySnapshot.docs()

Let me know how that works for you.

ReallySmallSoftware commented 5 years ago

I will take a look. I'm grateful for the amount of effort and diligence you have put into this and I'm going to pull it down and test with my apps. I'm not concerned about breaking changes if they are for the right reasons - and as you say, the version increment reflects this.

ReallySmallSoftware commented 5 years ago

Sorry - closing that was not my intention!

ReallySmallSoftware commented 5 years ago

@jggc As you are probably already aware, I have merged this having successfully tested against my apps. I have not released a new version yet. I am assuming you may want to test the merged version yourself first just to be sure but it did merge without any issues.

I have also made you a collaborator.