Dynamic layer using both f:image and token

[gavinr]

This contains a few additional test cases, one of which is failing, illustrating the issue in #1180 .... I think :)

@jptheripper @jwasilgeo @jgravois do you think this covers the issue in #1180?

[jgravois]

first you write the tests...
then you fix the tests...
and when you fix the tests, you squash the 🐛.

plenty of ways to skin a cat, but one would be for @jwasilgeo to PR against this branch and then merge this one to master once Travis is happy.

[jwasilgeo]

Thanks for starting up this testing effort, @gavinr. Much appreciated. I have pushed 4b84d4c which gets your new tests and all tests to pass.

This is the same change I described here for DynamicMapLayer.js.

... proposed change was to delete those override lines in initialize ...

[jgravois]

I have pushed 4b84d4c which gets your new tests and all tests to pass.

there's just one problem. @gavinr only wrote tests for the token.

if you want to remove the override entirely you'd need to get the test below passing too, but I still recommend doing that in a follow up PR.

it('should be able to pass a request for a direct image through a proxy', function () {
  layer = L.esri.dynamicMapLayer({
    url: url,
    f: 'image',
    proxy: './proxy.ashx'
  });
  var spy = sinon.spy(layer, '_renderImage');
  layer.addTo(map);
  expect(spy.getCall(0).args[0]).to.match(new RegExp(/\.\/proxy.ashx\?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 the updated info in your comment, that helps clarify a few things after temporarily trying out your test snippet.

[jwasilgeo]

@jgravois @gavinr please take another look when you can. The f override in initialize was trimmed down to now only override f to be 'json' if a proxy is present in this.options. This means we should be able to alleviate #1180 and the DynamicMapLayer.js can now use both f: 'image' and a token.

I plan to deal with adding a test(s) and adding DynamicMapLayer.js support for proxy and f: 'image' in a separate effort, where I will also try to make sure the same behavior and tests are included with the closely related ImageMapLayer.js.

[jgravois]

👍

[jwasilgeo]

@jgravois @gavinr while looking ahead to the future work I mentioned just above, I re-remembered that DynamicMapLayer.js never had passing tests to support f: 'json' and a proxy, despite that this was the only behavior of the layer if a proxy was provided in the options. My mega comment over here points to a commented-out test that maybe we could get to pass. Unfortunately I'm at a standstill with this one after tinkering with it a bit. Can I get a 2nd pair of 👀 on this please?

The changes proposed in this PR, for f: 'image', don't have any influence over that branch of the logic in the _renderImage method.

[jgravois]

DynamicMapLayer never had passing tests to support f: 'json' and a proxy
The changes proposed in this PR, for f: 'image', don't have any influence over that branch of the logic

that is an improvement that would be better tackled in its own PR. if you're stuck on it, I've found that just commenting out the test and explaining where you're at in a draft PR is a good way to frame the discussion.

[jwasilgeo]

That's a good plan to me, @jgravois. Thanks again for taking the time to talk this through with us. Otherwise are you both still feeling good about this PR as it is?