search

ionic-team / ionic-native-google-maps

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

WIP: Split out classes into separate files #103

Closed adamduren closed 5 years ago

adamduren commented 5 years ago

Apologies if this PR seems a bit extreme. I only split out the classes into multiple files so that it is more manageable going forward. Maps are an important part of the ecosystem and I would like to help the effort to make this the go to Google Mapping plugin

adamduren commented 5 years ago

I split out the classes into multiple files so that it is easier to digest this project for contributors. I am interested in helping maintain both the ionic-native plugin as well as the cordova one in any way that I can.

wf9a5m75 commented 5 years ago

I tried couple of times, but separating files cause loading problem on browser platform.

adamduren commented 5 years ago

Yea, I did some more testing and I see the chance for issues. I will spend some more time on this one. Marked in title as WIP. Hopefully I can have something in the next day or so.

wf9a5m75 commented 5 years ago

Actually, I prefer one big file to synchronize master and v5 branches . Because just copy and past then modify the code. Separating files increase my work.

adamduren commented 5 years ago

What if we were to have file parity between the master and v5 branches? That is something I can take on as well and verify both version build and load correctly. In the end v5 will be being merged into master correct? From an engineering perspective it would be better to have the files split out than one big 30,000 line file.

wf9a5m75 commented 5 years ago

In the end v5 will be being merged into master correct?

I guess no. Why? Ask to the ionic native team at https://github.com/ionic-team/ionic-native

wf9a5m75 commented 5 years ago

I appreciate if you create test case instead of spreading files.

And I aware tslint of v5 branch is not perfect. Some points you figured out in PR#100 did not discovered by tslint (npm run test). I appreciate if you fixed the settings. (I think configuration mistake or something).

adamduren commented 5 years ago

In the end v5 will be being merged into master correct?

I guess no. Why? Ask to the ionic native team at https://github.com/ionic-team/ionic-native

Because v5 will come out of beta and then there will be no reason for it not to be master. For maintenance reasons there should be a v4 fork. As far as writing tests that is something that I can do however it normally makes sense to have multiple spec files that correspond to the class under test which would also go along with splitting out the single index.ts into multiple files. We can put this MR on hold for now until we have test coverage to feel good about moving forward wit it.

wf9a5m75 commented 5 years ago

The reason of branch name v5 is current @ionic-native series use v4.xxx. That's why the ionic native team decided v5 branches, I guess. Since I'm not member of ionic team, I don't know the details. (And I'm not interested in about that)

@ionic-native/google-maps has been spin out project from @ionic-native repository for many reasons. However this repository is still under the ionic-team. That's why I keep file structure is the same with ionic-native repo as much as possible.

@ionic-native/google-maps is just a wrapper plugin for me. I add/modify new properties/methods/classes after I implemented new features in cordova-plugin-googlemaps. That's why one big file does not become a big problem for me so far.