refactor: moved wait and analysis stores to pinia - #1918
Conversation
To support potential future work to move to Vue 3, Vuex needs to be replaced with Pinia. I started with a couple of easy stores (analysis and wait). Pinia convention recommends a flatter file structure than was needed with Vuex. However, I still kept the stores separated by folder to facilitate breaking them out into actions and getters if desired. Pinia-converted stores are kept in /stores to differentiate them as more stores are moved over. Reset logic was also reconfigured in store/index to account for the new stores not being in the old Vuex store tree. Signed-off-by: Tracy Workman <tworkman08@gmail.com>
pedrolamas
left a comment
pedrolamas
left a comment
There was a problem hiding this comment.
Reviewed base cdefdcbd → head ac1f4f80. All 13 wait/* and analysis/* call sites are converted — no dangling $typedDispatch('wait/...'), $typedGetters['wait/...'] or dispatch: strings remain, and no spec touches these stores. Pinia activation ordering is correct: Vue.use(PiniaVuePlugin) runs at import of src/stores/pinia.ts, setActivePinia fires in the root beforeCreate during new Vue({ pinia }), and every useXStore() call site is lazy and post-mount (appInit() runs after $mount). vue-demi is already in pnpm-workspace.yaml allowBuilds, so its postinstall switch runs. No crash-level bugs found.
Assessment: performance and maintainability going forward
Performance — essentially neutral, slightly negative on bundle. Pinia 2 on Vue 2.7 sits on the same reactivity core (vue-demi → Vue 2.7's reactive/computed), so waits tracking behaves identically to the Vuex getters it replaces — I traced the hasWait method-style getter and confirmed the render watcher still collects state.waits at call time, so there's no reactivity regression. Costs added: ~5 kB gzip for pinia plus a duplicated vue-demi (see inline comment on the lockfile), Vuex stays resident for the whole migration so both systems ship simultaneously, and useStore() resolution moves onto the socket hot path and widget render paths. @vue/devtools-api should tree-shake in prod via Vite's process.env.NODE_ENV replacement, but worth confirming in a bundle report. None of this is material at Fluidd's scale; the honest summary is "no perf win, small perf tax, paid for architectural reasons".
Maintainability — right direction, but the seam needs hardening before it scales. Two stores out of 28 is a good, low-risk pilot and the conversions are faithful. The concern is the cost of the in-between state, which will last many PRs:
- The hand-maintained reset registry fails silently (see
src/store/index.ts) — this is the one thing I'd fix before merging, since it's the mechanism that will actually break as migration proceeds. - Two side-effect dispatch mechanisms now coexist in
socketActions.ts, which will multiply onceprinter/filesmove. - Migrated state loses Vuex's dev-only
strictmutation guard, with no Pinia equivalent. src/storevssrc/storesis a standing typo hazard (one character apart, both resolve).
Recommend landing this with an explicit migration order, a typed reset registry, and a decision on whether NotifyOptions.dispatch/commit are being retired — otherwise the half-migrated state becomes the steady state.
| state: (): AnalysisState => ({ | ||
| status: null, | ||
| }), | ||
| getters: { |
There was a problem hiding this comment.
low. Empty getters: { } block is noise.
Also worth noting status is write-only: nothing in src/ reads useAnalysisStore().status, and serverAnalysisStatus (its only writer) has zero callers. Pre-existing dead state, but the migration is a good moment to delete it rather than port it.
There was a problem hiding this comment.
Removed getters, status, serverAnalysisStatus as suggested. the only outstanding action on useAnalysisStore() is now onAnalysisProcess - im wondering if it should be left as-is or folded into files, which is the only consumer, and dropping the analysis store entirely.
| import { TinyColor } from '@ctrl/tinycolor' | ||
| import dbKey from '@/util/db-key' | ||
| import { useWaitStore } from '../../stores/wait' | ||
|
|
There was a problem hiding this comment.
low. Relative '../../stores/wait' here, but @/stores/analysis in src/store/index.ts (and '../stores/wait' on the line above it) — three styles for the same target.
Given src/stores/ sits one character away from the existing src/store/, mixed relative paths between the two trees are a real typo hazard: ../stores/… mistyped as ../store/… resolves to a different, existing directory and silently imports the wrong thing. Suggest standardising on @/stores/… everywhere, and possibly a less collision-prone directory name (src/pinia/).
There was a problem hiding this comment.
Yup, that's my error, ill standardize to '@/'
I see valid concern here about store/stores. I took the naming from Pinia's migration guide regarding stores. End state @/stores makes a more descriptive folder structure for maintainability long term and fits the style of the rest of the code base where folders are generally descriptive by function rather than library. How would you like to proceed?
There was a problem hiding this comment.
Thank you, please continue with @/stores plan, and keep an eye to make sure there are no leftovers on @/store after migration.
|
Hi @tworkman08, thank your for this Pull Request. This looks quite promising and I like it as a first approach to Pinia! I've pointed my Claude to this and it has posted a few comments from a first review that will need to be resolved. I would also prefer to avoid |
Thanks for the detailed reply @pedrolamas, I definitely wanted to start slow and easy. especially since I made assumptions during the initial pr write and wanted to get feedback on the approach. I'll take a look at the reviews above and make the necessary updates shortly. |
Addresses feedback from review on moved stores wait and analysis to pinia pr. File structure and naming now follows the reccomended structure from Pinia, rather than the previous vuex model: https://pinia.vuejs.org/cookbook/migration-vuex.html#Restructuring-Modules-to-Stores Standardized paths to @/ rather than relative paths. Signed-off-by: Tracy Workman <tworkman08@gmail.com>
Adressing review feedback: - Created plugin to register pinia stores on creation + added import.meta.glob to eagerly instansiates Stores. creating a single source of truth without having to manually add stores to a list. - updated reset function to resetPiniaStores() iterating the actual plugin registry above, so a store can't be migrated to pinia and silently skipped on reset - dded check in reset when a reset key matches neither a Vuex nor a pinia store. - Replace serverAnalysisProcess's unconditional .then() with a declarative pinia: option on NotifyOptions, mirroring dispatch:/commit: and staying overridable by callers this is expandable to typedDispatch/commit as well - Lazily cache useWaitStore() in WebSocketClient and StateMixin instead of resolving it on every socket message/render - Dropped dead code in stores/analysis.ts: onAnalysisStatus, it's corresponding caller in SocketActions.ts, and Analysis Status which was set, but never read. Signed-off-by: Tracy Workman <tworkman08@gmail.com>
pinned ^0.14.10 which is compatible with both echarts and pinia. signed-off-by: Tracy Workman <tworkman08@gmail.com>
To support potential future work to move to Vue 3, Vuex needs to be replaced with Pinia. I started with a couple of easy stores (analysis and wait). Pinia convention recommends a flatter file structure than was needed with Vuex. However, I still kept the stores separated by folder to facilitate breaking them out into actions and getters if desired. Pinia-converted stores are kept in /stores to differentiate them as more stores are moved over. Reset logic was also reconfigured in store/index to account for the new stores not being in the old Vuex store tree. Signed-off-by: Tracy Workman <tworkman08@gmail.com>
Addresses feedback from review on moved stores wait and analysis to pinia pr. File structure and naming now follows the reccomended structure from Pinia, rather than the previous vuex model: https://pinia.vuejs.org/cookbook/migration-vuex.html#Restructuring-Modules-to-Stores Standardized paths to @/ rather than relative paths. Signed-off-by: Tracy Workman <tworkman08@gmail.com>
Adressing review feedback: - Created plugin to register pinia stores on creation + added import.meta.glob to eagerly instansiates Stores. creating a single source of truth without having to manually add stores to a list. - updated reset function to resetPiniaStores() iterating the actual plugin registry above, so a store can't be migrated to pinia and silently skipped on reset - dded check in reset when a reset key matches neither a Vuex nor a pinia store. - Replace serverAnalysisProcess's unconditional .then() with a declarative pinia: option on NotifyOptions, mirroring dispatch:/commit: and staying overridable by callers this is expandable to typedDispatch/commit as well - Lazily cache useWaitStore() in WebSocketClient and StateMixin instead of resolving it on every socket message/render - Dropped dead code in stores/analysis.ts: onAnalysisStatus, it's corresponding caller in SocketActions.ts, and Analysis Status which was set, but never read. Signed-off-by: Tracy Workman <tworkman08@gmail.com>
pinned ^0.14.10 which is compatible with both echarts and pinia. signed-off-by: Tracy Workman <tworkman08@gmail.com>
https://github.com/tworkman08/fluidd into pinia-refactor signed-off-by: Tracy Workman <tworkman08@gmail.com>
signed-off-by: Tracy Workman <tworkman08@gmail.com>
Remove outdated and redundant comments to enhance code understanding. Update the `usePiniaStore` helper's comment to accurately describe its behavior with unmatched Vuex namespace/actions. Apply minor formatting adjustments for improved readability. Signed-off-by: Tracy Workman <tworkman08@gmail.com>
|
@pedrolamas - I believe I've addressed the concerns you've presented so far. I would love if you'd take a look at the updated branch and let me know if you have further feedback. |
Signed-off-by: Pedro Lamas <pedrolamas@gmail.com> # Conflicts: # pnpm-lock.yaml # src/plugins/socketClient.ts # src/store/config/actions.ts
Bundle size report (gzip)
121 chunks compared, 49 changed. Sizes are gzip, matching what nginx serves. |
Invert ownership of the analysis socket call: the store awaits SocketActions itself instead of the API layer naming a handler by string. This drops NotifyOptions.pinia, the store registry lookup and its helpers, and keeps socketActions.ts free of store imports (a typed callback there would have been circular). Reset is now typed too: the root reset payload is (keyof RootState)[], so a stale module name is a compile error rather than a silent skip. Pinia stores are reset in full when no payload is given, and explicitly by their owner otherwise, which removes the need to eagerly instantiate every store at startup. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013YXJV5dJMQHee58crfm46M Signed-off-by: Pedro Lamas <pedrolamas@gmail.com>
Time analysis is fired per file without awaiting, so a Moonraker error or a mid-flight socket drop surfaced as one unhandled rejection per file. Log through consola instead; the user-facing toast already comes from the global socket error handler. Also adds specs for resetPiniaStores, covering both an options store and a setup store providing its own $reset. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018n862D2eUnE6eV22wAydSS Signed-off-by: Pedro Lamas <pedrolamas@gmail.com>
The memoised waitStore getters in StateMixin and WebSocketClient cached what is only a map lookup, duplicating the same pattern across two files with two different typings. Call useWaitStore() directly instead, and bind a local only where one function uses a store more than once. Also drops Moonraker.Analysis.StatusResponse, orphaned when serverAnalysisStatus was removed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018n862D2eUnE6eV22wAydSS Signed-off-by: Pedro Lamas <pedrolamas@gmail.com>
2 participants
To support potential future work to move to Vue 3, Vuex needs to be replaced with Pinia. I started with a couple of easy stores (analysis and wait). Pinia convention recommends a flatter file structure than was needed with Vuex. However, I still kept the stores separated by folder to facilitate breaking them out into actions and getters if desired. Pinia-converted stores are kept in /stores to differentiate them as more stores are moved over. Reset logic was also
reconfigured in store/index to account for the new stores not being in the old Vuex store tree.
Signed-off-by: Tracy Workman tworkman08@gmail.com