Particular / ServicePulse

Production monitoring for distributed systems.
https://docs.particular.net/servicepulse/
Other
34 stars 27 forks source link

Refactor remaining composables used for global state management to use Pinia stores to resolve potential flaky acceptance tests caused by shared module-level state #1905

Open TravisNickels opened 4 months ago

TravisNickels commented 4 months ago

Describe the suggested improvement

Is your improvement related to a problem? Please describe.

Our current Vue.js application has some remaining composables that maintain state at the module level. While this approach simplifies state sharing across components, it introduces significant issues in our acceptance tests conducted with Vitest and JSDOM. Specifically, the shared module-level state could lead to flaky tests due to state persistence between test runs.

NOTE: The application already uses Pinia stores. See https://github.com/Particular/ServicePulse/tree/master/src/Frontend/src/stores. This issue is about completing the migration to Pinia but emphasizes the importance of this work to help with automated testing efforts.

Testing challenges when using module-level state

Impact

Describe the suggested solution

Refactor our composables to use Pinia stores instead of maintaining state at the module level. Pinia is the recommended state management library for Vue.js that offers reactive and typesafe stores. By using Pinia:

Many, if not all, of the "composables" in the Frontend\src\composables folder can be either moved to Pinia stores or refactored into general functions, with their names adjusted to not start with "use".

With testability in mind, composables should primarily only be used for organizing shared functionality and logic, rather than for global state management.

Advantages of Using Pinia

Describe alternatives you've considered

  1. Do not use JSDOM for acceptance testing and do not directly instantiate the app, instead use a headless browser. However, the idea behind using a virtual DOM (JSDOM) instead of a real browser is to have acceptance tests that run way faster than tests that require spinning a whole headless browser
  2. Refactor Composables to Avoid Module-Level State

Refactor your composables so that they don't rely on module-level state. Instead, initialize state within functions or use a factory pattern. However this creates state everytime the useComposable function is invoked, defeating the purpose of having state that is global.

Before (Module-Level State in Composable):

// myComposable.js
import { ref } from 'vue';

// Module-level state (shared across imports)
const globalState = ref(0);

export function useMyComposable() {
  return {
    globalState,
  };
}

New state instance per call (not useful for state that is meant to be shared across components)

// myComposable.js
import { ref } from 'vue';

export function useMyComposable() {
  const state = ref(0);
  return {
    state,
  };
}

Additional Context

Main offenders that urge this refactoring:

johnsimons commented 4 months ago

Pinia is the recommended approach for managing state in ServicePulse

I don't remember seeing any mention of this, has this been RFCed?