Esri / esri-leaflet

A lightweight set of tools for working with ArcGIS services in Leaflet. :rocket:
https://developers.arcgis.com/esri-leaflet/
Apache License 2.0
1.6k stars 798 forks source link

Cannot read property 'tagName' of undefined #1107

Closed HarelM closed 6 years ago

HarelM commented 6 years ago

Steps to reproduce the error:

  1. Hard to tell, related to zoom in and zoom out when using dynamic map layer

What happens is [X]. Error is thrown by leaflet code I was expecting [Y]. No error should be thrown

The following error is what I get:

ERROR TypeError: Cannot read property 'tagName' of undefined
    at NewClass._initImage (leaflet-src.js:9057)
    at NewClass.onAdd (leaflet-src.js:8941)
    at NewClass.onAdd (esri-leaflet-debug.js:2177)
    at NewClass._layerAdd (leaflet-src.js:6487)
    at NewClass.whenReady (leaflet-src.js:4367)
    at NewClass.addLayer (leaflet-src.js:6549)
    at NewClass.addTo (leaflet-src.js:6429)
    at NewClass._renderImage (esri-leaflet-debug.js:2339)
    at NewClass.<anonymous> (esri-leaflet-debug.js:2864)
    at NewClass.<anonymous> (esri-leaflet-debug.js:1599)

Layer address: https://gisn.tel-aviv.gov.il/arcgis/rest/services/IView2Ortho2017/MapServer Debug scrennshot image

Might be related to leaflet 1.3...? Let me know if I can assist in debugging.

jgravois commented 6 years ago

i'm at a loss for how this._url could be undefined intermittently.

a few folks have reported more desirable behavior by forming requests such that the raw image is returned immediately (instead of getting back JSON that points to the output in a cached directory).

 L.esri.dynamicMapLayer({
    url: 'https://services.arcgisonline.com/arcgis/rest/services/Specialty/Soil_Survey_Map/MapServer',
    f: "image"
}).addTo(map);

that may resolve this error too.

HarelM commented 6 years ago

Actually, this._url is not null. see the screenshot - it is the address of the image and the tagname is not defined since it is not an object... I'll see if the above workaround can fix it, nevertheless.

pmacMaps commented 6 years ago

Is it worth updating the API reference to mention the format (f) option and the various formats that can be returned from the server? If people know that image is a choice for format, and may produce more desirable results, I think it would be helpful to users of the library.

HarelM commented 6 years ago

Nope, adding f: "image" doesn't help :-/ image

jgravois commented 6 years ago

i can reproduce the error panning/zooming a few times in http://jsbin.com/jedamif/edit?html,output.

this is what's going on:

  1. when f: "json" is set (or nothing, since it's the default), for some reason this particular service intermittently returns a bogus 200 response that is missing the url of the output image entirely.
// 20180709092545
// https://gisn.tel-aviv.gov.il/arcgis/rest/services/IView2Ortho2017/MapServer/export?bbox=3860012.600630188%2C3782104.1595505225%2C3886287.829103217%2C3774288.473408363&size=1375%2C409&dpi=96&format=png24&transparent=true&bboxSR=3857&imageSR=3857&f=json
{
  "href": "",
  "width": 0,
  "height": 0,
  "extent": {
    "xmin": 3860012.600630187,
    "ymin": 3774288.4734083619,
    "xmax": 3886287.8291032175,
    "ymax": 3782104.1595505215,
    "spatialReference": {
      "wkid": 102100,
      "latestWkid": 3857
    }
  },
  "scale": 0
}
  1. setting f: "image" doesn't help anything because the server still throws errors intermittently producing images. in this case, some requests are cancelled entirely and we receive a response that doesn't include a CORS header, screenshot 2018-07-09 09 31 16 screenshot 2018-07-09 09 30 52

when making the same request directly in the browser, the error message below is displayed.

Could not access any GIS Server machines. Please contact your system administrator.

When a service is behaving like this, Esri Leaflet could (and should) do a better job of catching the errors.

That said, it'd be a lot more helpful to fix the service itself. are you guys able to communicate with the folks that administer this instance of ArcGIS Server? I'd love to get them some assistance and resolve the root configuration problem.

jgravois commented 6 years ago

Is it worth updating the API reference to mention the format (f) option and the various formats that can be returned from the server?

@pmacMaps the API reference does list f as a constructor option, but the description is currently cryptic to the point of being useless. a pull request that either lists other options explicitly or links to relevant REST documentation would definitely be welcome.

HarelM commented 6 years ago

@jgravois Thanks for looking into this and finding the issue that fast! Please let me know when there's a new release of esri-leaflet that includes this fix so that I'll be able to update my site with it as I think that fixing the servers will take more time than making this library a bit more robust (From the issue I linked it seems that there is more than one server with this issue). If you think this this fix won't be release in the next week or so please kindly let me know and I'll decide how to proceed with the usage of this library in my site. Thanks again for all the hard work!

jgravois commented 6 years ago

@HarelM since you said you're using webpack, would you be willing to point directly at master and test the patch i come up with prior to an official release?

HarelM commented 6 years ago

Sure, add the relevant instructions and I'll be happy to test it.

jgravois commented 6 years ago

fixed in 317c536.

if you make sure to set f:"json in your DynamicMapLayer constructor now, 'Cannot read property 'tagName' of undefined' should no longer make it to the console in your app when those installs of ArcGIS Server 💩 the bed.

HarelM commented 6 years ago

How can I configure webpack to use master? I don't see the "compiled" results in the repository...? npm install https://github.com/Esri/esri-leaflet.git --save?

jgravois commented 6 years ago

instead of loading your dependencies from a tagged version in the npm registry:

// package.json
"dependencies": {
    "esri-leaflet": "^2.2.0"

you can also just point directly at a GitHub repo:

"dependencies": {
    "esri-leaflet": "Esri/esri-leaflet" // #master will be pulled by default

https://docs.npmjs.com/files/package.json#github-urls

HarelM commented 6 years ago

But I still need to build it locally in order for this to work, right?

HarelM commented 6 years ago

Seems like I can't compile it locally for some reason, something to do with missing profiles for rollup or something. I've looked at travis-ci but I don't know where the esri-leaflet-debug.js file is shipped to if it gets shipped. @jgravois Can you send me esri-leaflet-debug.js file and I'll test the fix.

jgravois commented 6 years ago

You should be able to configure Webpack to compile our raw ES6 source, but i guess you'd have to use babel-loader for that.

https://www.dropbox.com/s/hfoijhd6tr11zao/esri-leaflet-debug.js?dl=0 https://www.dropbox.com/s/9int13dp3i0u6a8/esri-leaflet-debug.js.map?dl=0

HarelM commented 6 years ago

This seems to have solve the issue. Thanks!! Please let me know when a release containing this fix & #1106 will be available (#1106 is not listed in the release notes of 2.2.0).

jgravois commented 6 years ago

the fix in #1106 was shipped with v2.2.0, i probably should have, but i didn't list it in the release notes because the code change was upstream.

i'll think about publishing another release of this library after i've had a chance to get #1084 landed.

jgravois commented 6 years ago

v2.2.1 has been released.

HarelM commented 6 years ago

Yay! Thanks! Keep up the good work!