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

feat: handle updates to Source tileJsonSource prop more gracefully #914

Merged
merged 5 commits into from
Nov 24, 2020
Merged

feat: handle updates to Source tileJsonSource prop more gracefully #914

merged 5 commits into from
Nov 24, 2020

Conversation

henrinormak
Copy link
Contributor

@henrinormak henrinormak commented Nov 19, 2020

In mapbox 1.12.0, a feature was released which allows changing the URL/tiles props on a vector source without removing it from the map. This is much more performant, especially in cases where the map undergoes several back-to-back changes to its tiles URL (after a few rapid changes, mapbox-gl starts to have issues when constantly removing/re-adding layers/sources).

In this PR, I've:

  • Updated dependency to mapbox-gl to be the latest (I had to make some type changes to get the build to pass, ideally these wouldn't be needed, but I'm hoping these could be resolved at a later date)
  • Modified the Source component to use the new setUrl and setTiles APIs when dealing with a vector source. I kept the old behaviour as a fallback, as raster tile sources for example didn't get this new feature.

When testing in a local setup with an app that does many tiles property changes, this new approach is significantly more performant.

@mklopets mklopets requested a review from alex3165 November 23, 2020 14:24
src/source.ts Outdated
const hasNewSourceUrl =
tileJsonSource.url !== this.props.tileJsonSource.url;

if (hasNewSourceUrl && this.props.tileJsonSource.url) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this doesn't permit unsetting the URL – not sure if there would be a valid use case for it, but is it intentional here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Mapbox types seem to indicate that the url cannot be "unset", i.e the functions don't take undefined as an argument - for that I guess one would have to change the entire source. I did now however make this check more explicit about undefined.

@henrinormak
Copy link
Contributor Author

henrinormak commented Nov 24, 2020

Added an example as well, based it on the example from Mapbox, with the extension that you can switch the mirror for the tiles, which also tests the changes in this PR.

The problem I had originally is not surfaced in this demo, fortunately, as likely the variety in the tiles URL is too small (i.e if it had query parameters that changed etc, then the problem would surface), I did manage to reproduce it by adding a random query parameter to it (something like t=Math.random()), after toggling the layer a few times, the performance of reloading the tiles after the button clicked started going down - this only happened with the old Source implementation and I couldn't reproduce it with the updated Source.

@mklopets mklopets merged commit d802870 into alex3165:master Nov 24, 2020
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.

2 participants