allow override of 'isModern'

[spoilsportmotors]

Add an option to let users decide to not use geoJSON, even if the
underlying service supports it. Occasionally handy when ArcGIS Server
has geoJSON geometry problems.

[jgravois]

thanks so much for this!

it's not a good idea to set isModern to true by default (because of old feature services)

my first instinct is to just include a conditional check within the metadata response handler and skip setting the property for users 'if' they already set it themselves.

hope that makes sense.

[spoilsportmotors]

It does make more sense; let me got back and change the behavior.

[spoilsportmotors]

I updated PR #935; for back-compatibility, I think the option needs to be left explicit & set to 'true'. The metadata check will explicitly set isModern correctly if there isn't server support (older servers).

[jgravois]

I think the option needs to be left explicit & set to 'true'

why exactly? i see two distinct benefits to not assuming that a feature service endpoint supports geojson.

1. support for geojson was introduced in ArcGIS Server 10.3.1. a substantial number of customers are running something older.
2. since all feature services support native esri json, switching from f=json to f=geojson (when appropriate) is a seamless transition. the inverse will result in requests that fail.

[spoilsportmotors]

The submitted PR does exactly what you're asking; it'll cheerfully ask for f=geojson only when the service supports it, just like before. The only difference is that someone (ok, me), can specify isModern: false in the options, and I'll get f=json on all the requests.

The current behavior will always use f=geojson if it can; the PR keeps that behavior. Anyone upgrading won't see a change in how requests are made.

If instead we change the default behavior to use f=json, then people will see a difference in the requests made, and they'd have to change their code to allow the use of geoJSON.

I'm happy to change the default value - but opting out of geoJSON seemed a better choice than forcing people to change their code to opt-in. Regardless, if the service doesn't support geoJSON, the requests will use f=json.

[jgravois]

The current behavior will always use f=geojson if it can; the PR keeps that behavior

i ran a quick test using your branch and confirmed that this is not true.

// 10.01 ArcGIS Server instance that doesn't support f=geojson
var fl = L.esri.featureLayer({
  url:'http://sampleserver1.arcgisonline.com/ArcGIS/rest/services/Demographics/ESRI_Census_USA/MapServer/5'
}).addTo(map);

because the initial queries are executed before the metadata response is retrieved, the fact that you are setting isModern as the default property for FeatureManager alters the behavior and causes f=geojson requests to go out when the property is not set manually by the developer.

[spoilsportmotors]

Gaaah.

IDK why I didn't see that here, but ... I get it. Thanks for your patience; update in the works.

[spoilsportmotors]

Yay. Tests pass.
I built out a hokey test app here, and it does what's expected when you pass isModern: false in the options, and also the property is missing - so no changes are needed to existing apps that want to use geoJSON.

Thanks for your patience - I didn't fully grok the startup process when a new layer is created & added (honestly, probably still don't.)

[jgravois]

thanks for your contribution!

it may have been intentional on your part, but the actual commits you pushed are associated with @ahwarren and not @spoilsportmotors.