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

Persist sources and layers when basemap style changes #60

Merged

Conversation

stuartlynn
Copy link
Contributor

This PR fixes a bug where user defined layers and sources got overwritten when the basemap styleURL changed.

To fix this I added some extra logic to the MapLibre component that

  1. When the map style loads, we take note of what layers and sources it defines.
  2. When the style parameter changes, we compare the current list of sources and layers on the map to the ones on the old basemap style and take note of any which are not part of the old basemap style
  3. Then when the new style loads, we reload each of the sources and layers that got deleted.

I also added an example page which shows this in effect, mostly for testing.

Would be great to get a few eyes on this PR as the logic is a bit complex and I want to make sure this doesn't have any unintended consequences or side effects

@vercel
Copy link

vercel bot commented Sep 20, 2023

Someone is attempting to deploy a commit to a Personal Account owned by @dimfeld on Vercel.

@dimfeld first needs to authorize it.

Copy link
Contributor

@dabreegster dabreegster left a comment

Choose a reason for hiding this comment

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

I think this approach is quite clever and simple, and nicely localized to one place. I have some smaller svelte-maplibre projects I can change to take advantage of this for some more testing if it's useful

src/lib/MapLibre.svelte Outdated Show resolved Hide resolved
);
sourcesToReAddAfterStyleChange = {};
for (const id of nonStyleSourceIds) {
sourcesToReAddAfterStyleChange[id] = oldMapStyle.sources[id];
Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking about cases where the paint or layout prop of a Layer component gets modified. Doing the work here based on map.getStyle() is nice, because it'll pick up the current state before changing style.

One sanity check though -- we don't need to make defensive copies with structuredClone or similar here? I don't think so, because map.getStyle() will return new things after the setStyle below, and the user-managed sources/layers won't be there. The maplibre API isn't terribly clear about ownership/lifetime of source/layer specifications...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I dont think so either. Especially as the only thing we are storing from this entire function call are the layersToReAddAfterStyleChange and sourcesToReAddAfterStyleChange, objects which are newly constructed each time. We can throw in a structuredClone just in case if we want to be 100% sure. The style shouldn't change often enough that we need to worry about keeping this code too performant. Miight be better to be defensive?

@vercel
Copy link

vercel bot commented Sep 22, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
svelte-maplibre ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 22, 2023 8:47pm

@dimfeld
Copy link
Owner

dimfeld commented Sep 22, 2023

I should be able to take a real look at this and your other PR later today. Thanks!

Edit: actually will more likely be over the weekend. Thanks for your patience :)

@dimfeld dimfeld merged commit ffd53aa into dimfeld:master Sep 25, 2023
@dimfeld
Copy link
Owner

dimfeld commented Sep 25, 2023

Ok, released all the recent PRs as v0.4.0. Thanks @stuartlynn and @dabreegster for the PRs and reviews!

@dabreegster
Copy link
Contributor

Just spotted maplibre/maplibre-gl-js#2587 (comment), possibly a simpler implementation

@dimfeld
Copy link
Owner

dimfeld commented Mar 15, 2024

Just spotted maplibre/maplibre-gl-js#2587 (comment), possibly a simpler implementation

Thanks, yeah that does look like a nice solution.

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