Refactor Map state to be more declaritive

[stuartlynn]

I wanted to propose a potential code refactor that might make dealing with the map a little more declarative and less imperative and decouple the map based code from the specifics of the application.

Basically what I think we can do is

• Remove the Map singleton from the store and instead just store the relevant state of the map location, selection and layer definitions in stores
• Have layers, selection variables and location be inputs to the map component
• Write code that inspects layers on update, decides what has changed ( layer added / removed / style updated) and makes the appropriate changes to the map object (now stored as a variable on the component instead of as a singleton)
• Do something similar for base map changes which should now be a little easier as we have the layer definitions stored explicitly in the store and can re-apply them after the basemap changes nukes the style.
• Write a wrapper component that interacts with the stores to get the relevant information and passes it as regular inputs (either as props on the Map component or as child elements / slots for layers)

There are a few advantages to this approach

• Simplifies the data flow of the app by placing all state in stores rather than some state being managed by the map internally and some by the stores.
• It explicitly keeps track of the layers which are active on the map so that other parts of the application can inspect them update them without having to know anything about the Map
• It means that if we ended up wanting to have a second map on the site (which might be useful for comparing two regions or to have two variables shown side by side) we could easily reuse the logic by simply setting up stores for it's layers etc ( potentially sharing some layer definitions with the existing map) and dropping in an new
• It gets us closer to how svelte-maplibre works so that if it reaches feature parity with what we have in the app, we can just replace it, reducing the amount of code we have to maintain.

[dabreegster]

Huge +1 to this. Let us know how we can help!

Do something similar for base map changes which should now be a little easier as we have the layer definitions stored explicitly in the store and can re-apply them after the basemap changes nukes the style

Brilliant idea, I love this approach! The alternative is making every single component listen for the appropriate map lifecycle event and try to re-create state. This would be so much simpler.

It gets us closer to how svelte-maplibre works

Do you think we should go for this intermediate refactor first, or attempt to cutover to svelte-maplibre directly? My inclination is the former, since we can iterate faster without needing to fix things upstream. But ultimately I'd like to use much of the same code in other Turing projects, so common libraries somewhere would be nice.

If it makes the refactor any easier, this ATIP codebase is logically 3 different tools at this point -- the scheme sketching bit, the browse page, and a critical issue entry page. The critical issue page and the homepage (used for the scheme sketching) are the simplest / most self-contained. The browse page is big (many layers) but conceptually pretty simple. And the sketch tool is the most complicated, because it's managing both previously drawn objects and letting you select one, hide it from that main drawing layer, and enable a tool to edit its geometry. The selection logic being tied to behavior in the sidebar is also complicated there.

[dabreegster]

I found an old branch where I tried to use svelte-maplibre here. I'll start a list of blockers/problems just to write 'em down somewhere, but please let me know if that's more helpful to stick in a new issue!

• If a Svelte component with a source like <GeoJson> hot-reloads, the source isn't torn down and recreated properly
• We need to upstream something like https://github.com/acteng/atip/blob/main/src/lib/common/PmTiles.svelte for pmtiles support
• If you bind:map, you get a map that's maybe not loaded yet, and so downstream code using the map directly has to deal with that everywhere. This is maybe an argument for not touching the map directly and only using small little components to handle the reactive updates.
• Some annoyances with <Popup let:features>. The features may be empty, even in cases where that's never true at runtime. Because we can't use TypeScript directly in the inline HTML section of svelte components, it stops us from using nice simple inline things sometimes. (Because we can't use ! to insist on non-nullness)
• https://github.com/dimfeld/svelte-maplibre/issues/36

None of these were blockers/huge; I just haven't taken the time yet to work on upstream fixes

[stuartlynn]

Thanks for the link here. I am tempted to say that we should fork the svelte-maplibre module, point these projects to our forked copy, try and resolve these issues in our version, try to get those fixes accepted back in to the offical branch and then revert back to using that.

I think thats the proper way of doing things but I sympathise with wanting to go with the move faster approatch.

I might spend a day seeing if I can find fixes for the issues you proposed and if not we can do a more imediate refactor on our code?

Sound good?

[stuartlynn]

Also thats annoying about typescript in the svelte templates... I didn't realise that was a thing :-(

[dabreegster]

@dimfield to confirm, but PRs in svelte-maplibre have gone through very quickly. If we provide high-quality fixes to the things above (none of which are at all unique to us), I imagine we could fix a few things upstream first. As we migrate more things here to svelte-maplibre, I'm sure we'll uncover more things and can keep iterating.

From a refactoring perspective here, it might be nice to chunk it up and completely cutover some of the simpler pages first?

[stuartlynn]

Yeah that makes sense for sure... though I wonder if tackling the most compliceted page first might help us more quickly indentify if there are any massive gotachas we can't currently anticipate. Would be good to know about these before we go through the work of doing the simpler pages and then potentially find that we have to abandon svelte-maplibre.

Both have their merits :-)

[dabreegster]

https://github.com/acteng/atip/tree/svelte_maplibre_v2 starts to port to svelte-maplibre for the browse page. There are some small problems I'm hitting; will either file issues or send quick PRs to fix. But overall it's working fine, and the code doesn't change too much from how it was before. I do have ideas for more refactoring that this might unlock, though

[dabreegster]

We've now cutover to svelte-maplibre! I've filed #359 to track smaller bugfixes and improvements that this unblocks