Convert store.ts into multiple Pinia stores
#141
Conversation
jjnesbitt
changed the title
store.ts into multiple Pinia stores
| const activateColor = "#008837"; | ||
|
|
||
| const map = mapStore.getMap(); | ||
| map.getLayersOrder().forEach((mapLayerId) => { |
There was a problem hiding this comment.
I'm getting a typescript error for mapLayerId implicitly having an any type here, as well as in a handful of other places (all errors in attached txt file)
pinia-store-typescript-errors.txt
| let xmin, xmax, ymin, ymax, srs = undefined; | ||
| if (currentFrame.raster) { | ||
| const bounds = currentFrame.raster.metadata.bounds; | ||
| ({ xmin, xmax, ymin, ymax, srs } = bounds); |
There was a problem hiding this comment.
I'm getting a typescript error Type 'string' is not assignable to type 'undefined'. for the assignment of srs here
| const currentStyle = styleStore.selectedLayerStyles[styleId]; | ||
| currentStyle.visible = layer.visible | ||
|
|
||
| // TODO: Move this conditional functionality into `addLayer`, and directly call addLayerFrameToMap there |
There was a problem hiding this comment.
Is this TODO intentionally left for future work?
| // Must pass actual network object into setNetworkDeactivatedNodes, not computed prop | ||
| // TODO: Fix in the store function itself | ||
| if (networkStore.currentNetwork) { | ||
| selectedNodes.value = [] | ||
| setNetworkDeactivatedNodes(currentNetwork.value, []) | ||
| networkStore.setNetworkDeactivatedNodes(networkStore.currentNetwork, []) | ||
| } | ||
| } | ||
|
|
||
| function toggleSelected() { | ||
| if (currentNetwork.value) { | ||
| // Must pass actual network object into setNetworkDeactivatedNodes, not computed prop | ||
| // TODO: Fix in the store function itself |
There was a problem hiding this comment.
Are these TODOs intentionally left for future work?
| import { useLayerStore, useNetworkStore, useStyleStore } from "@/store"; | ||
|
|
||
| const networkStore = useNetworkStore(); | ||
| const layerStore = useLayerStore(); | ||
| const styleStore = useStyleStore(); |
There was a problem hiding this comment.
I like how in this file, the calls of all the "use store" functions are directly below the import of those functions. Could we do it consistently like this in all the other files? I think it makes it easier to read and understand what's going on.
| loadingCharts, | ||
| availableCharts, | ||
| currentChart, |
There was a problem hiding this comment.
I'm curious why charts are rolled into the analysis store instead of on their own. Is this just for consolidation of what might have been two very sparse stores?
| const rasterTooltipDataCache = computed(() => layerStore.rasterTooltipDataCache); | ||
| const clickedFeature = computed(() => mapStore.clickedFeature); |
There was a problem hiding this comment.
Why make these computed variables? Why not just refer to the store when accessing these variables? (Same goes for selectedLayers further down)
| export const useProjectStore = defineStore('project', () => { | ||
| const networkStore = useNetworkStore(); | ||
| const analysisStore = useAnalysisStore(); | ||
| const mapStore = useMapStore(); | ||
| const layerStore = useLayerStore(); | ||
| const panelStore = usePanelStore(); | ||
| const appStore = useAppStore(); | ||
| const styleStore = useStyleStore(); |
There was a problem hiding this comment.
Is there ever a concern of a circular-import sort of problem when you need to reference other stores in a store like this? Is there a hierarchy of which stores are loaded first?
This PR converts our use of global store via vue refs, to instead use pinia stores. These stores allow better organization, and makes it easier to track what state is used and where.
Since this is a large change, I recommend going through the commits one at a time. I structured them in a well scoped way. It's possible that there are still some rough edges, as I tried to mainly focus on keeping this PR scoped to reorganization, rather than lower level refactoring.