Use Vuex to manage error modal #1225
Conversation
anttimaki
commented
Feb 16, 2024
anttimaki
commented
- Main benefit is that emitted errors no longer need to bubble all the way to the App.vue to get shown. This should prevent situations where refactoring components distrupts the emit chain and leaves the error unshown
- Refactors the modal to a single standalone component
- Fixes the odd naming where R2Error's "name" was stored in variable called "message", and "message" was stored in "stack" variable
- Unifies some logged messages from "name\n-> message" format to "[name]: message" format
|
There was a problem hiding this comment.
Haven't finished reviewing yet but immediate thought is (and has been for a few other PRs already): we should stop expanding the ModalsModule.ts to become a mega-monolith of 329 different modal use cases, most of the modals have nothing to do with each other so it's a bit silly to store their state in the same module.
I don't think it should be considered a blocker for this PR necessarily, but I don't like the trend I've seen with how the code has evolved in that module over time. It's not even namespaced so it could easily lead to unintentional conflicts in the future at this rate.
src/store/modules/ModalsModule.ts
Outdated
| state: (): State => ({ | ||
| downloadModModalMod: null, | ||
| errorModalError: null, | ||
| isCategoryFilterModalOpen: false, | ||
| isDownloadModModalOpen: false, | ||
| isErrorModalOpen: false, | ||
| isGameRunningModalOpen: false, | ||
| }), |
There was a problem hiding this comment.
what's the benefit in tracking the error state & modal open state separately? as far as I can tell those two fields are supposed to always be in sync anyway, could we not just check if the error exists? would eliminate any possibility of the two desyncing & potentially leading to weird UI bugs down the line.
There was a problem hiding this comment.
Just to stay consistent with downloadModModalMod and disableModModalMod (not pictured). But I agree, it's unnecessary.
src/store/modules/ModalsModule.ts
Outdated
| @@ -38,6 +55,21 @@ export default { | |||
| state.isDownloadModModalOpen = true; | |||
| }, | |||
|
|
|||
| openErrorModal: function( | |||
There was a problem hiding this comment.
I feel like this does more than just opens the modal so the name could be a bit misleading, would it make sense to just call this something like handleError?
- Main benefit is that emitted errors no longer need to bubble all the way to the App.vue to get shown. This should prevent situations where refactoring components distrupts the emit chain and leaves the error unshown - Refactors the modal to a single standalone component - Fixes the odd naming where R2Error's "name" was stored in variable called "message", and "message" was stored in "stack" variable - Unifies some logged messages from "name\n-> message" format to "[name]: message" format
anttimaki
force-pushed
the
vuex-error-modal
branch
from
80ff3c7
to
781f23c
Compare
@MythicManiac do you want to review again or am I good to merge? |
| if (error instanceof R2Error) { | ||
| LoggerProvider.instance.Log(LogSeverity.ERROR, `[${error.name}]: ${error.message}`); | ||
| } else { | ||
| const msg = error.logMessage || `[${error.error.name}]: ${error.error.message}`; | ||
| LoggerProvider.instance.Log(error.severity, msg); |
