ismyrnow / leaflet-groupedlayercontrol

Leaflet layer control with support for grouping overlays together.
MIT License
274 stars 141 forks source link

removeLayer method throws an error #49

Open ttsvetko opened 7 years ago

ttsvetko commented 7 years ago

Here it the code-fragment for 'removeLayer':

 removeLayer: function (layer) {
    var id = L.Util.stamp(layer);
    var _layer = this._getLayer(id);
    if (_layer) {
      delete this.layers[this.layers.indexOf(_layer)]; // `this.layers` should be replaced with `this._layers`
    }
    this._update();
    return this;
  }

Instead of this.layers should be this._layers i guess.

ismyrnow commented 7 years ago

You're totally right about that. Would someone mind opening a pull request with the fix?

ttsvetko commented 7 years ago

I can do later

ForgottenLords commented 7 years ago

Calling delete on the array index is not sufficient, as it is later iterated over; delete removes the index from the array, causing the deleted index's value to be 'undefined' when expecting an object. Instead, you must splice the item from the array to leave the indexes continuous.

The current pull request #50 is insufficient. I had included a fix in my pull request #46 (an unrelated feature request).

karlitos commented 6 years ago

Please, accept the PR to fix the issue caused by using delete on array.

newmanw commented 6 years ago

Possible to merge this pull request and release a new version to npm?

ChrisSoper1 commented 6 years ago

It looks like the pull request has been accepted, but the NPM package has not been updated