search

ionic-team / ionic-native-google-maps

Google maps plugin for Ionic Native
Other
221 stars 125 forks source link

WIP: Discussion, Simplifying Ionic Native Google Maps Plugin #206

Closed adamduren closed 5 years ago

adamduren commented 5 years ago

@wf9a5m75 below is an example of simplifying the ionic-native-google-maps library to rely on the cordova plugin vs reimplementing functionality. Want to get your thoughts as it can lead to less code to maintain, less bugs, more consistency.

adamduren commented 5 years ago

Overall this will be a breaking change but the benefits of increased code sharing I believe outweigh the downsides. We would need to communicate this to the community.

ionic-native-google-maps and cordova-plugin-googlemaps are two projects that I use heavily. I would be willing to undertake this refactor for ionic-native so that it is easier to maintain which will then allow us to focus more effort on cordova-plugin-googlemaps

wf9a5m75 commented 5 years ago

I don't want to provide two syntaxes. Please don't change a lot.

adamduren commented 5 years ago

@wf9a5m75 there is more complexity around here than I thought. I am going to leave this open while I think on it more.

As mentioned, in the end I would like the ionic-native plugin to be pretty minimal and delegate to cordova-plugin-googlemaps for most things.

wf9a5m75 commented 5 years ago

Thank you for creating PR, but I think you still need to learn both plugins.

The @ionic-native/google-maps plugin has original logic only for ionic, especially GoogleMaps.create() around, that's why I don't merge both plugins.

Also I have a plan for supporting ionic/capacitor. The capacitor platform requires different plugin base if the case don't use ionic-native. I don't want to re-create the maps plugin.

I faced various problems, and fixed them until today. Those days has formed current code. I appreciate your PR, but please don't make a lot of changes.

However creating a good discussion is always welcome.

wf9a5m75 commented 5 years ago

I only merged tsconfig.json file at this time.

adamduren commented 5 years ago

After further research I am understanding why the code is the way it is. Namely I didn't understand the BaseClass and BaseArrayClass being in the ionic plugin as well but now see that the behavior is customized for ionic native to do things like return Observables vs passing an event listener.