Skip to content

Conversation

@DoiMayank
Copy link

The following changes were made:

Updated the logic to default isModern to true for feature services.

Modified instances where isModern is used to ensure GeoJSON is requested by default.

@gavinr-maps
Copy link
Contributor

@DoiMayank what issue is this addressing? If this is addressing a bug, can you please provide a replication case for the bug. Thanks!

@DoiMayank
Copy link
Author

@gavinr-maps This PR addresses the performance issues discussed in #1372 and #1373. The main problem comes from requesting features before confirming if the service supports GeoJSON. This causes unnecessary overhead because browsers have to convert the data to GeoJSON when the service could handle it directly.

The solution is to make isModern the default for feature services since GeoJSON output has been supported in ArcGIS Server starting with version 10.4, which was deprecated in February 2022. By doing this, we avoid the extra processing on the client side and take advantage of the service’s capability to return GeoJSON directly, improving overall performance.

@gavinr-maps
Copy link
Contributor

@DoiMayank ok, thanks. So is the replication case in #1373 (copied below) the best test of this fix or something else?
image

@DoiMayank
Copy link
Author

@gavinr-maps I believe the solution proposed in #1376 is the best fix for this issue. I have implemented it in this PR, and @patrickarlt was also leaning in this direction.

@gavinr-maps
Copy link
Contributor

Ok, thanks. I'll discuss with Patrick the best way to test this in the context of #1376.

Copy link
Contributor

@patrickarlt patrickarlt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DoiMayank I think this might need some adjustments. In order to trigger the isModern behavior down in the service you'll need to set isModern to true when the service is created in https://github.com/Esri/esri-leaflet/blob/master/src/Layers/FeatureLayer/FeatureManager.js#L40. To do this you'll need to set isModern: true in the default options in https://github.com/Esri/esri-leaflet/blob/master/src/Layers/FeatureLayer/FeatureManager.js#L17-L28

After this change isModern will default to true everywhere and will never be undefined Which should simplify most of your remaining code because we no longer even need to check with the service if it supports GeoJSON or not. We assume it supports GeoJSON unless the user explictly tells us it does not.

If we want to resolve the potential performance issues in #1373 then we need to know ahead of time if the service supports GeoJSON which we can reasonably assume it does. Otherwise we have to wait for the metadata call before requesting any features which would be a major rewrite of FeatureManager.

@patrickarlt
Copy link
Contributor

Another interesting observation from this is that for services that do not support GeoJSON the object id field bust be OBJECTID or it likely wont work at all. This is because we don't wait for the metadata call on requests so a non-standard metadata field MIGHT be returned AFTER a query response comes back and we will get some strange behavior when converting to GeoJSON.

@patrickarlt
Copy link
Contributor

@DoiMayank I discussed this internally with the team. We all agreed that setting isModern to true bu default as described in #1403 (review) IF we also print a warning that service does not support GeoJSON and isModern is set to true the best place for this is probably in https://github.com/Esri/esri-leaflet/blob/master/src/Layers/FeatureLayer/FeatureManager.js#L78-L114

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants