Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Only load featurelayer tiles after metadata has been loaded #1372

Closed
wants to merge 1 commit into from

Conversation

markhepburn
Copy link

This can have a significant impact on performance for larger layers, where the initial detailed tiles are requested concurrently with the metadata request and thus loaded in arcgis format -- assuming isModern hasn't been manually set -- and need to be converted to geojson (which can choke a browser).

This is only a partial fix, as _setView can also trigger _update.

A potential fix for this is an instance variable to check whether metadata has loaded yet (tricky as metadata is requested in a subclass), or some proxy for this -- I have been using "_cells is empty or not", but there may be a better way, and I'm possibly missing cases as well.

This can have a significant impact on performance for larger layers,
where the initial detailed tiles are requested concurrently with the
metadata request and thus loaded in arcgis format -- assuming isModern
hasn't been manually set -- and need to be converted to geojson (which
can choke a browser).

This is only a partial fix, as _setView can also trigger  _update.
@gavinr-maps
Copy link
Contributor

gavinr-maps commented Sep 28, 2023

Thanks for the PR. Do you have some sample code that would demonstrate the issue you're fixing here?
Edit: Nevermind I see your comments in #1373 now!

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.

This fix looks good on the surface but it means that sub-classes of FeatureGrid are now responsible for calling the initial _update when they are ready to start loading features.

The issue is that there are TONS of cases were things will happen that will start loading features before we are ready. For example what if the user pans or zooms after loading a layer but before we know if it supports GeoJSON or not?

I'll have to give a think on this. I'm leaning towards just making isModern enabled by default now. f=geojson has been supported since ArcGIS Server 10.4 (source) which was officially deprecated in February 2022 (lifecycle doc) so I think we could probably make GeoJSON the default now and avoid this issue.

This would however be a breaking change and require a major version bump.

@markhepburn
Copy link
Author

markhepburn commented Oct 6, 2023

@patrickarlt Thanks for looking at this, that all makes sense.

One thing that would still be affected is the Esri/esri-leaflet-renderers; I found that early polygons are unstyled, until you zoom in or otherwise load more. It's a tricky problem though, that's why I only called this a partial fix!

Update: I just realised that the custom renderers repo was archived a few hours ago, so perhaps that's not a priority!

@patrickarlt
Copy link
Contributor

@markhepburn closing this in favor of #1376. Thanks for bringing this up. In the meantime I would just manually set isModern.

Yes @gavinr-maps and I decided to archive Esri/esri-leaflet-renderers since it introduces a lot of additional support requests and expands the scope of the library with the expectation that we will support lots of other things which we really don't have the capacity for.

@patrickarlt patrickarlt closed this Oct 9, 2023
@markhepburn
Copy link
Author

@patrickarlt Quite understand, thanks for taking the time to look at this.

What's the current recommended platform for incorporating esri feature service layers (with styling), out of curiosity? We picked Leaflet for various reasons including excellent React bindings, but it's getting harder to support non-image formats such as ESRI or pbf.

@patrickarlt
Copy link
Contributor

What's the current recommended platform for incorporating esri feature service layers (with styling), out of curiosity?

This would be the official ArcGIS Maps SDK for JavaScript which supports everything you can configure in the map viewer and ArcGIS Pro and as a bonus also gives you full web map support which is too large in scope for us to build for Esri Leaflet. Keep and eye out for some announcements this month with regard to some new releases which should hopefully make integration with 3rd party frameworks easier.

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