Fix crash caused when initing map with DOM element

[adamduren]

@wf9a5m75 Can you please review this PR? The plugin is currently crashing for me when passing a HTMLElement to the GoogleMaps.create().

[wf9a5m75]

You should use GoogleMaps.create('#id_name') instead of passing HTMLElement.

[wf9a5m75]

Unfortunately, not acceptable this PR at this time.

I guess the reason of crashing is that you are trying to create a map to HTMLElement which does not have parent node. You can not do that.

[adamduren]

@wf9a5m75 I tested these changes locally and I can confirm that it works when passing an HTMLElement

[adamduren]

In response to:

You should use GoogleMaps.create('#id_name') instead of passing HTMLElement.

As I said I can confirm it works when I pass an HTMLElement. Also if what you say is true then why is there a check for instanceof HTMLElement that instantiates the plugin with it?

[wf9a5m75]

Also if what you say is true then why is there a check for instanceof HTMLElement that instantiates the plugin with it?

Because original code writer (I guess ionic team member) wrote so that. However this causes tons of problems. That's why I recommend GoogleMaps.create('#id_name') way currently.

[wf9a5m75]

Can you believe that? Many ionic user write <div id="map_canvas"></div> in multiple page.

Guess what if there are <div id="map_canvas"></div> in two pages, then which DOM element is returned if the code execute document.getElementById('map_canvas')?

Ionic uses fade-in/fade-out when you change page through tab or side menu. It means the two pages are visible at the moment.

Then document.getElementById('map_canvas') returns pageA, but after the soon, pageA is gone, then map is removed automatically.

In order to resolve this problem, I recommend GoogleMaps.create('#id_name') way currently.

[wf9a5m75]

And in order to prevent other errors, this plugin waits the element is stable at least 100px x 100px.

[adamduren]

The way I am currently using it and it is working for me. Note the use of ElementRef.

import {
  Component,
  ElementRef,
  Input,
  OnChanges,
  OnDestroy,
  OnInit,
} from '@angular/core';
import {
  GoogleMap,
  GoogleMapOptions,
  GoogleMaps,
} from '@ionic-native/google-maps/ngx';
import { LaunchNavigator } from '@ionic-native/launch-navigator/ngx';

import { IAddress } from '@adamduren/schema';

@Component({
  selector: 'app-map',
  templateUrl: 'app-map.html',
})
export class MapComponent implements OnInit, OnChanges, OnDestroy {
  @Input()
  public markerType: 'default' | 'radius' = 'default';

  @Input()
  public item: { address: IAddress; latitude: number; longitude: number };

  @Input()
  public title: string;

  @Input()
  public navigationButtonText: string = 'Get Directions';

  @Input()
  public showNavigationButton: boolean = true;

  private map: GoogleMap;
  private mapOptions: GoogleMapOptions = {
    controls: {
      zoom: false,
      myLocation: true,
    },
    camera: {
      zoom: 10,
    },
  };

  constructor(
    private launchNavigator: LaunchNavigator,
    private el: ElementRef,
    private googleMaps: GoogleMaps,
  ) {}

  public ngOnInit(): void {
    const el = this.el.nativeElement as HTMLElement;
    const mapEl = el.querySelector('.map-container');
    this.map = this.googleMaps.create(mapEl, this.mapOptions);

    if (this.item) {
      this.map.animateCamera(this.getAnimationOptions());

      if (this.markerType === 'radius') {
        this.map.addCircle({
          center: this.getLatLng(),
          radius: 150,
          strokeColor: '#327EFF32',
          strokeWidth: 2,
          fillColor: '#327EFF32',
        });
      } else {
        this.map.addMarker({
          position: this.getLatLng(),
        });
      }
    }
  }

  public ngOnChanges(): void {
    if (this.item && this.map) {
      this.map.animateCamera(this.getAnimationOptions());
    }
  }

  public ngOnDestroy(): void {
    this.map.destroy();
  }

  private getAnimationOptions() {
    return {
      target: this.getLatLng(),
      duration: 1000,
      zoom: 15,
    };
  }

  private getLatLng() {
    return {
      lat: this.item.latitude,
      lng: this.item.longitude,
    };
  }
}

[adamduren]

The above way allows me to always target the correct element without running the risk of selecting multiple elements by using the same id.

[wf9a5m75]

If you want to do that, do it. As far as I tested on many cases of many users, I don't recommend it.

[adamduren]

Can you provide a case where passing an Element is problematic and explain why? I would imagine that it would be the preferred way of doing it.

[wf9a5m75]

Unfortunately I can not provide good example.

You may believe all ionic user use your way.

    const el = this.el.nativeElement as HTMLElement;
    const mapEl = el.querySelector('.map-container');
    this.map = this.googleMaps.create(mapEl, this.mapOptions);

But it's not true. Many ionic user use

let mapEl = document.getElementById('map-container');
this.map = this.googleMaps.create(mapEl, this.mapOptions);

[adamduren]

Just because many do it that way doesn't mean it's the right way nor am I claiming my way is the best way.

You said:

If you want to do that, do it. As far as I tested on many cases of many users, I don't recommend it.

Followed by:

Unfortunately I can not provide good example.

If you have tested many cases that led you to believe that it is problematic then you should be able to explain why that is the case.

[wf9a5m75]

The reason of I can not give a good example is that I faced many problems during debugging many user's project.

Unfortunately I don't believe all ionic developer write code as you.

[wf9a5m75]

And the problem occurs rarely, not always. It's really depends on ionic version, project structure, user behaviors, developer's code.

As I said, if you believe your case is the best, yes, please do it in your project. I don't hold back you. You write code for only your case. But I need to write code for many users.

[wf9a5m75]

By the way, some points you figured out are correct, such as clearInterval(). I appreciate for that.

[adamduren]

I believe I have addressed your comments and I appreciate your feedback. I understand that you do not agree with passing an Element however this library allows for it and the code that is currently on the v5 branch is broken as I pointed out domNode is not defined. It should be element. I have corrected that issue.

With that in mind can this be accepted? If not what additional changes need to happen to make it in?

[wf9a5m75]

As I said some points you figured out are correct. I fixed those points of the v5 branch code. Could you double check the latest commit? And if there is another points, please let me know.

I will check the master branch (for ionic v3) during the meantime.

[adamduren]

I will be away for the evening. What remain are the changes that still need to be implemented. I will check back in the morning.

[wf9a5m75]

Could you pull v5 branch at once?

Deleting unreachable code in commit #cc8a82c causes problem. Because those code for cordova-plugin-googlemaps. I fixed it and push it already, but your commit was faster.

[wf9a5m75]

Um, it seems v5 branches test code is not perfect.