1180-image-and-token

[jwasilgeo]

1. This was initially related to Dynamic layer cannot use both f:image and token #1180 to allow both an optional token and f: image for a DynamicMapLayer instance. It seems that the conditional override of f in the initialize method was not allowing both of these options to persist. But, I still don't know what the original intent of this was and it would be easier to rest at night if we could find out. The similarly-structured ImageMapLayer does not do this in its initialize.

2. Please also reference changes introduced a few months ago for issue DynamicMapLayer and Proxy JSON returned results in Proxy Request ?undefined #1153 that involved related blocks of code in DynamicMapLayer's _requestExport. I'm leaving this as a breadcrumb here in case we find in the future that we need to add the same extra checks in ImageMapLayer's _requestExport.

3. Along the way I standardized similarities I noticed between DynamicMapLayer and ImageMapLayer in their usage of this.options vs params in their _buildExportParams and _requestExport methods.

4. Finally, I added an optional prepending of params.proxy when f is image for both layer types.
   I do not have a way to easily test this functionality. We may want to strike it from this PR and reconsider it in the future if it is reported as an issue that someone can't use both f: image and a proxy. Note that the appending of params.token was already successfully assumed to occur in the code.

   • https://github.com/Esri/esri-leaflet/compare/1180-image-and-token?expand=1#diff-a7be8ad7c53337df7dc83d076f320957R201-R204
   • https://github.com/Esri/esri-leaflet/compare/1180-image-and-token?expand=1#diff-92a727b8d8cc44ec622b0a0335471d15R207-R210

I can consider updating related tests once we have a chance to talk a bit about these changes.

I also added 3 new debug HTML pages to verify my work so far.

cc @jgravois @jptheripper

[jgravois]

I do not have a way to easily test this functionality.
I can consider updating related tests once we have a chance to talk a bit about these changes.

I'd say these changes are a great candidate for some TDD.

what you're proposing is adding support for manipulating DynamicMapLayer f:"image" requests with:

• a proxy
• a token

right now Travis is reporting a failing test. if you can fix that test and add two more then we'd be (reasonably) sure that the tweaks accomplish what you advertise and won't break any existing use cases.

this test would be a good one to copy/paste and modify. sound good?

[jptheripper]

Just to be clear, we are NOT pulling a /ImageServer as type image
we are pulling a dynamic /MapServer as f=image (as opposed to json) to prevent accessing the /arcgisoutput directory

[jwasilgeo]

@jptheripper, yes thanks for the clarification. While working on this I wanted to make the same/similar changes to the related ImageMapLayer.js for /ImageServer because they are quite similar in nature.

[jptheripper]

Perfect thank you. I really appreciate this.

[jwasilgeo]

@jgravois I tinkered with and tried to work on the existing test for a but, but in the end I found that can get the test in question passing again by reversing this change:
https://github.com/Esri/esri-leaflet/pull/1181/files#diff-a7be8ad7c53337df7dc83d076f320957L182-R181

Which doesn't quite make sense to me since token is available in both this.options and params at that point in time in the method _requestExport in the case of f: "json". It is added on to params just before in the method _buildExportParams (and to add to the uncertainty it came from this.service.options rather than from this.options (which have very similar properties)).

Thus (my bullet 3 above) I had hoped that by replacing this.options with params in _requestExport it would make reading through and understanding that method easier in the future in DynamicMapLayer and ImageMapLayer. Perhaps in the interest of time it may be simplest to reverse those changes?

---

what you're proposing is adding support for manipulating DynamicMapLayer f:"image" requests with:

• a proxy
• a token

That's correct (for my bullet 4 above), but now I'm thinking about dropping proxy support for f: "image" since this PR already introduces enough changes and I don't want to hold up #1180 too long unless someone else has more free time for writing new proxy tests (which I found commented out).

However, regarding adding token support for f: "image":

Note that the appending of params.token was already successfully assumed to occur in the code.

That's because _requestExport in the case of f: "image" was only using params (no this.options) and appeared to be doing it fine with the token available in the params object.

[jwasilgeo]

The more I ponder this, the more I see this PR is trying to do too much. Let's start with @gavinr's new unit tests in PR #1182 and go from there. Perhaps then we can ultimately focus on only what is needed to satisfy issue #1180. Again, that's described above as:

1. This was initially related to Dynamic layer cannot use both f:image and token #1180 to allow both an optional token and f: image for a DynamicMapLayer instance. It seems that the conditional override of f in the initialize method was not allowing both of these options to persist. But, I still don't know what the original intent of this was and it would be easier to rest at night if we could find out. The similarly-structured ImageMapLayer does not do this in its initialize.

My proposed change was to delete those override lines in initialize, but an alternative code change to consider could be @jptheripper's comment here.

[jgravois]

this PR is trying to do too much.

those are wise words. it will always be possible to change the behavior for proxies in a follow-up pass and limiting a pull request to tackling one thing at a time doesn't just make it easier to write, it makes it easier to read too.

[jwasilgeo]

Closing, because #1180 was fixed in PR #1182. We can refer to some of the info in this comment thread later when dealing with proxy support with f: 'image' in DynamicMapLayer and ImageMapLayer.