Dynamic Layers crash when minZoom or maxZoom specified

slead commented 7 years ago

This may be related to https://github.com/Esri/esri-leaflet/issues/910, which is marked as resolved, but I'm seeing this using the demo below, which uses Leaflet 1.0.3 and EsriLeaflet 2.0.7

When using minZoom or maxZoom on a Dynamic Layer, I'm seeing a fatal error. To reproduce:

zoom in until the States layer becomes visible at level 7
zoom out to level 8 so it switches off
pan the map

I'm seeing Uncaught TypeError: Cannot read property 'removeLayer' of null, and the map becomes unresponsive to further panning.

<html>
<head>
  <meta charset=utf-8 />
  <title>Simple DynamicMapLayer</title>
  <meta name='viewport' content='initial-scale=1,maximum-scale=1,user-scalable=no' />

  <!-- Load Leaflet from CDN-->
  <link rel="stylesheet" href="https://unpkg.com/leaflet@1.0.3/dist/leaflet.css" />
  <script src="https://unpkg.com/leaflet@1.0.3/dist/leaflet-src.js"></script>

  <!-- Load Esri Leaflet from CDN -->
  <script src="https://unpkg.com/esri-leaflet@2.0.7"></script>

  <style>
    body { margin:0; padding:0; }
    #map { position: absolute; top:20; bottom:0; right:0; left:0; }
  </style>
</head>
<body>
  <span id="zoom">Zoom in until the States become visible at level 7, then zoom out, and pan</span>
  <div id="map"></div>
<script>
  var map = L.map('map').setView([37.71, -99.88], 4);

  map.on("zoomend", function(){
    document.getElementById("zoom").innerHTML = "Current zoom level is " + this.getZoom();
  })

  L.esri.basemapLayer('Gray').addTo(map);

  L.esri.dynamicMapLayer({
    url: 'http://sampleserver1.arcgisonline.com/ArcGIS/rest/services/Demographics/ESRI_Census_USA/MapServer/',
    layers: [5],
    useCors: false,
    minZoom: 7
  }).addTo(map);
</script>

</body>
</html>

jgravois commented 7 years ago

ugh. thanks for the report.

and the map becomes unresponsive to further panning.

i can definitely reproduce the console error, but when i load this jsbin in Chrome 56, map state isn't affected.

// RasterLayer._update()
if (zoom > this.options.maxZoom || zoom < this.options.minZoom) {
  // _currentImage is still defined from previous display (incorrectly)
  if (this._currentImage) {
    this._currentImage._map.removeLayer(this._currentImage);
  }
  return;
}

you interested in taking a crack at fixing the bug?

jordanparfitt commented 7 years ago

I think this is fixed in master but not in 2.0.7

jgravois commented 7 years ago

we got a little sidetracked with other git configuration stuff in #917, but given that it was only 3 weeks ago, this still serves as a testament to how poor my memory is.

@jordanparfitt is correct that the problem is resolved in master, but it'd be nicer to nullify _currentImage entirely when appropriate instead of checking for _currentImage._map

if (this._currentImage && this._currentImage._map)

either way i'll tag a new release soon.

slead commented 7 years ago

Thanks guys. I'll close this for now and will re-test it when the next release is out.

jgravois commented 7 years ago

this fix is included in v2.0.8

jordanparfitt commented 7 years ago

I got around to doing some testing and I'm still getting the remove layer error on _currentImage.

// RasterLayer._update()
if (zoom > this.options.maxZoom || zoom < this.options.minZoom) {
  // _currentImage is still defined from previous display (incorrectly)
  if (this._currentImage) {
    this._currentImage._map.removeLayer(this._currentImage);
  }
  return;
}

You mentioned nullifying _current image instead of inserting this line:

if (this._currentImage && this._currentImage._map)

Did the nullify part get inserted into 2.0.8?

jgravois commented 7 years ago

Did the nullify part get inserted into 2.0.8?

yup. its here.

i just retested @slead's repro case here (and yours in https://github.com/Esri/esri-leaflet/issues/910#issuecomment-274619531) with v2.0.8 and everything looked fine to me. if you are seeing errors, please feel free to log a new issue. i'd be happy to check into it.

jordanparfitt commented 7 years ago

My bower file was just jacked up. It works great. Thanks for the quick response.

On Fri, May 19, 2017 at 12:32 PM, john gravois notifications@github.com wrote:

Did the nullify part get inserted into 2.0.8?

yup. its here https://github.com/Esri/esri-leaflet/blob/v2.0.8/src/Layers/RasterLayer.js#L240

i just retested @slead https://github.com/slead's repro case here (and yours in #910 (comment) https://github.com/Esri/esri-leaflet/issues/910#issuecomment-274619531) with v2.0.8 and everything looked fine to me. if you are seeing errors, please feel free to log a new issue. i'd be happy to check into it.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Esri/esri-leaflet/issues/927#issuecomment-302778431, or mute the thread https://github.com/notifications/unsubscribe-auth/ABLNpkFJSRoDyHLQuTf-96FcYkz303l3ks5r7eA7gaJpZM4MCiId .

Esri / esri-leaflet

Dynamic Layers crash when minZoom or maxZoom specified #927