search

ionic-team / ionic-native-google-maps

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

BaseClass constructor doesn't make sense #106

Closed adamduren closed 5 years ago

adamduren commented 5 years ago

I'm not sure I understand why the constructor is written as so.

https://github.com/ionic-team/ionic-native-google-maps/blob/c717aea6377de9662761f7f088826a23767d1e14/src/%40ionic-native/plugins/google-maps/index.ts#L1459-L1464

should it be this instead?:

 constructor(_objectInstance?: any) { 
   if (!_objectInstance) { 
     _objectInstance = new (GoogleMaps.getPlugin().BaseClass)(); 
   } 
   this._objectInstance = _objectInstance; 
 }
adamduren commented 5 years ago

The this. in the if branch seems unintended, unless I'm missing something.

adamduren commented 5 years ago

It could also be written as:

 constructor(_objectInstance?: any) { 
   this._objectInstance = _objectInstance ||  new (GoogleMaps.getPlugin().BaseClass)();
 }

or even:

  constructor(protected _objectInstance: any = new (GoogleMaps.getPlugin().BaseClass)()) {}
wf9a5m75 commented 5 years ago

I prefer the person who send a PR instead of blaming if you find a mistake.

adamduren commented 5 years ago

It's not a matter of blame. I apologise if it came across that way. It was meant to be a question to see if there was something that I am missing.

Creating a pull request for a misunderstanding would be a waste of time.

wf9a5m75 commented 5 years ago

Fixed. And released as @ionic-native/google-maps@5.0.0-beta.25. Thank you for pointing out.

adamduren commented 5 years ago

Thank you. I'm working on a method to be able to test the ionic-native version of the plugin. I've got a prototype working. I still need to document how it works but will be presenting something in the next couple of days.