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

Fix e.isSourceLoaded to check if specific source is loaded #11393

Merged
merged 14 commits into from
Jan 25, 2022

Conversation

avpeery
Copy link
Contributor

@avpeery avpeery commented Jan 12, 2022

e.isSourceLoaded is intended to return true if a source has no outstanding network requests (see docs for isSourceLoaded in the Map Data Event). Previously, e.isSourceLoaded used this.loaded() from style.js to determine its value. this.loaded() in src/style/style.js however checks all sources to see if they are loaded rather than that specific source id. This fix uses the logic of map.isSourceLoaded to check if that specific source id is loaded.

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • manually test the debug page
  • apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'
  • add an entry inside this element for inclusion in the mapbox-gl-js changelog: <changelog>Fix e.isSourceLoaded to check if specific source has loaded.</changelog>

@avpeery avpeery changed the title Fixes e.isSourceLoaded to check if specific source is loaded Fix e.isSourceLoaded to check if specific source is loaded Jan 12, 2022
@avpeery avpeery linked an issue Jan 13, 2022 that may be closed by this pull request
Copy link
Contributor

@asheemmamoowala asheemmamoowala left a comment

Choose a reason for hiding this comment

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

The fix looks good, but is missing unit tests. There are similar tests in geojson_source.test.js that could be amended to test all the events from a setData call.

I tested against debug/geojson_update.html and found the following sequence of sourcedata events for a GeoJSON source:

  1. sourceDataType: metadata: On first being added to the map and after the worker has loaded inline data or fetched and loaded from remote data.
  2. sourceDataType: content: on subsequent calls to setData and after the worker has loaded inline data or fetched and loaded from remote data.
  3. sourceDataType: undefined for each tile required to complete rendering the current frame.

e.isSourceLoaded is false for every one of these emitted events until the last tile request is completed.

Copy link
Contributor

@asheemmamoowala asheemmamoowala left a comment

Choose a reason for hiding this comment

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

Approved pending an additional unit test with GeoJsonSource#setData showing that isSourceLoaded changes to true.

@avpeery
Copy link
Contributor Author

avpeery commented Jan 20, 2022

@asheemmamoowala Thanks for the review! I added in a call tosetData in the previous test case. If the source is not added to the map before calling setData, isSourceLoaded is not included in the 'data' event response (which is being set here: https://github.com/mapbox/mapbox-gl-js/blob/avpeery/fix-e.isSourceLoaded/src/source/geojson_source.js#L323). While isSourceLoaded is set here: https://github.com/mapbox/mapbox-gl-js/blob/avpeery/fix-e.isSourceLoaded/src/style/style.js#L708. Let me know if this makes sense and if this needs further conversation.

@avpeery avpeery merged commit 83bde34 into main Jan 25, 2022
@avpeery avpeery deleted the avpeery/fix-e.isSourceLoaded branch January 25, 2022 00:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

e.isSourceLoaded checks if all source caches are loaded
2 participants