Dynamic layer cannot use both f:image and token

[jptheripper]

• Browser and version:

Chrome 51

• Version of Leaflet (L.version):

0.7.7, 1.0.0?

• Version of esri Leaflet (L.esri.VERSION):

1.0.4, 2.0.3, master?

Steps to reproduce the error:

1. create map
2. add server authenticated service that requires password.
3. add parameter f:'json'
4. add parameter token: response.token

What happens is the image parameter is ignored and request is sent with type f=json

I was expecting f=image to be respected. Feature service only loads half the time due to internal load balancer not having request on /arcgisoutput (which is why we need to use the f=image parameter)

https://jsbin.com/hiqequg/edit?html,output

[jwasilgeo]

Some more background and historical info would be helpful here from previous contributors to ./src/Layers/DynamicMapLayer.js.

It appears to me that this is acting as designed in:

• esri-leaflet/src/Layers/DynamicMapLayer.js

  Lines 23 to 25 in 6cf7587

  	if ((options.proxy || options.token) && options.f !== 'json') {
  	options.f = 'json';
  	}

• esri-leaflet/src/Layers/DynamicMapLayer.js

  Line 177 in 6cf7587

  _requestExport: function (params, bounds) {

[jgravois]

theelse block below needs an options.token check similar to the if block above it, except it needs to optionally append that value as a parameter to the original url.

esri-leaflet/src/Layers/DynamicMapLayer.js

Lines 194 to 197 in 6cf7587

	} else {
	params.f = 'image';
	this._renderImage(this.options.url + 'export' + Util.getParamString(params), bounds);
	}

besides the service in @jptheripper's jsbin, you can also test with the one below.

username: user1
password: user1

http://sampleserver6.arcgisonline.com/arcgis/rest/services/Wildfire_secure_ac2/MapServer/export?token=foo&...

[jptheripper]

This is absolutely perfect. We can confirm once this functionality is applied that f='image' and token work together as desired.

[jptheripper]

Sorry, I am new to the whole process. How do we get the change applied to the source code?

[jgravois]

How do we get the change applied to the source code?

pull requests are always welcome.

in this case, a new test similar to the one below would be extremely helpful.

esri-leaflet/spec/Layers/DynamicMapLayerSpec.js

Lines 359 to 367 in c313044

	it('should be able to request an image directly from the export service', function () {
	layer = L.esri.dynamicMapLayer({
	url: url,
	f: 'image'
	});
	var spy = sinon.spy(layer, '_renderImage');
	layer.addTo(map);
	expect(spy.getCall(0).args[0]).to.match(new RegExp(/http:\/\/services.arcgis.com\/mock\/arcgis\/rest\/services\/MockMapService\/MapServer\/export\?bbox=-?\d+\.\d+%2C-?\d+\.\d+%2C-?\d+\.\d+%2C-?\d+\.\d+&size=500%2C500&dpi=96&format=png24&transparent=true&bboxSR=3857&imageSR=3857&f=image/));
	});

[jwasilgeo]

@jgravois thanks for input. And what should we do to this block that overrides on construction?

esri-leaflet/src/Layers/DynamicMapLayer.js

Lines 23 to 25 in 6cf7587

	if ((options.proxy || options.token) && options.f !== 'json') {
	options.f = 'json';
	}

[jptheripper]

Yes I was about to comment that needs to be dealt with as well. image type should not be overridden just because there is a token or proxy

[jwasilgeo]

Should it be dropped altogether? Not sure what the original intent was. Thanks!

[jgravois]

good catch @jwasilgeo. I forgot all about that.

to implement this new feature you'd need to remove the options.token check in the override.

if you want to take a stab at implementing the proxy logic for f: 'image' too, then you could get rid of it altogether.

Not sure what the original intent was.

maybe Pat figured that if folks wanted to work with secure layers they could just use f: 'json'.
I'm not sure.

[jptheripper]

In our test (we only use image, with and without token) simply commenting out the override worked for us. I imagine that may break other cases. Is it possible to just change the override to

if (((options.proxy || options.token) && options.f !== 'json')&&(!(options.token&& options.f == 'image'))) { options.f = 'json'; }

[jptheripper]

So, (because i am new to all this), how long does this take to propagate into the CDN

[gavinr]

@jptheripper in order for the fix in that Pull Request to become available for your use on the CDN, we must publish a new release. We intend to cut a new release soon.

[gavinr]

@jptheripper https://github.com/Esri/esri-leaflet/releases/tag/v2.3.3

[timothycouchhsi]

Thanks a ton! This is great. And you were so quick to respond :)