Browse Source

ref(ui): Make AlertStore state readonly (#72329)

Scott Cooper 9 months ago
parent
commit
c867b63f6f
2 changed files with 39 additions and 28 deletions
  1. 29 16
      static/app/stores/alertStore.spec.tsx
  2. 10 12
      static/app/stores/alertStore.tsx

+ 29 - 16
static/app/stores/alertStore.spec.tsx

@@ -4,8 +4,7 @@ jest.mock('sentry/utils/localStorage');
 
 describe('AlertStore', function () {
   beforeEach(function () {
-    AlertStore.alerts = [];
-    AlertStore.count = 0;
+    AlertStore.init();
   });
 
   describe('addAlert()', function () {
@@ -20,9 +19,9 @@ describe('AlertStore', function () {
         type: 'info',
       });
 
-      expect(AlertStore.alerts).toHaveLength(2);
-      expect(AlertStore.alerts[0].key).toEqual(0);
-      expect(AlertStore.alerts[1].key).toEqual(1);
+      expect(AlertStore.getState()).toHaveLength(2);
+      expect(AlertStore.getState()[0].key).toEqual(0);
+      expect(AlertStore.getState()[1].key).toEqual(1);
     });
 
     it('should not add duplicates when noDuplicates is set', function () {
@@ -39,23 +38,27 @@ describe('AlertStore', function () {
         noDuplicates: true,
       });
 
-      expect(AlertStore.alerts).toHaveLength(1);
+      expect(AlertStore.getState()).toHaveLength(1);
     });
   });
 
   describe('closeAlert()', function () {
     it('should remove alert', function () {
-      AlertStore.alerts = [
-        {key: 1, message: 'foo', type: 'error'},
-        {key: 2, message: 'bar', type: 'error'},
-        {key: 3, message: 'baz', type: 'error'},
-      ];
+      const alerts = [
+        {message: 'foo', type: 'error'},
+        {message: 'bar', type: 'error'},
+        {message: 'baz', type: 'error'},
+      ] as const;
+      for (const alert of alerts) {
+        AlertStore.addAlert(alert);
+      }
 
-      AlertStore.closeAlert(AlertStore.alerts[1]);
+      expect(AlertStore.getState()).toHaveLength(3);
+      AlertStore.closeAlert(AlertStore.getState()[1]);
 
-      expect(AlertStore.alerts).toHaveLength(2);
-      expect(AlertStore.alerts[0].key).toEqual(1);
-      expect(AlertStore.alerts[1].key).toEqual(3);
+      const newState = AlertStore.getState();
+      expect(newState).toHaveLength(2);
+      expect(newState).toEqual([alerts[0], alerts[2]]);
     });
     it('should persist removal of persistent alerts', function () {
       const alert = {
@@ -67,7 +70,17 @@ describe('AlertStore', function () {
 
       AlertStore.closeAlert(alert);
       AlertStore.addAlert(alert);
-      expect(AlertStore.alerts).toHaveLength(0);
+      expect(AlertStore.getState()).toHaveLength(0);
     });
   });
+
+  it('returns a stable reference from getState', () => {
+    AlertStore.addAlert({
+      message: 'Bzzzzzzp *crash*',
+      type: 'error',
+    });
+
+    const state = AlertStore.getState();
+    expect(Object.is(state, AlertStore.getState())).toBe(true);
+  });
 });

+ 10 - 12
static/app/stores/alertStore.tsx

@@ -4,7 +4,7 @@ import {createStore} from 'reflux';
 import {defined} from 'sentry/utils';
 import localStorage from 'sentry/utils/localStorage';
 
-import type {CommonStoreDefinition} from './types';
+import type {StrictStoreDefinition} from './types';
 
 type Alert = {
   message: React.ReactNode;
@@ -20,31 +20,29 @@ type Alert = {
 };
 
 interface InternalAlertStoreDefinition {
-  alerts: Alert[];
   count: number;
 }
 interface AlertStoreDefinition
-  extends CommonStoreDefinition<Alert[]>,
+  extends StrictStoreDefinition<Alert[]>,
     InternalAlertStoreDefinition {
   addAlert(alert: Alert): void;
   closeAlert(alert: Alert, duration?: number): void;
-  init(): void;
 }
 
 const storeConfig: AlertStoreDefinition = {
-  alerts: [],
+  state: [],
   count: 0,
 
   init() {
     // XXX: Do not use `this.listenTo` in this store. We avoid usage of reflux
     // listeners due to their leaky nature in tests.
 
-    this.alerts = [];
+    this.state = [];
     this.count = 0;
   },
 
   addAlert(alert) {
-    const alertAlreadyExists = this.alerts.some(a => a.id === alert.id);
+    const alertAlreadyExists = this.state.some(a => a.id === alert.id);
     if (alertAlreadyExists && alert.noDuplicates) {
       return;
     }
@@ -84,8 +82,8 @@ const storeConfig: AlertStoreDefinition = {
     // intentionally recreate array via concat because of Reflux
     // "bug" where React components are given same reference to tracked
     // data objects, and don't *see* that values have changed
-    this.alerts = this.alerts.concat([alert]);
-    this.trigger(this.alerts);
+    this.state = this.state.concat([alert]);
+    this.trigger(this.state);
   },
 
   closeAlert(alert, duration = 60 * 60 * 7 * 24) {
@@ -102,12 +100,12 @@ const storeConfig: AlertStoreDefinition = {
     }
 
     // TODO(dcramer): we need some animations here for closing alerts
-    this.alerts = this.alerts.filter(item => alert !== item);
-    this.trigger(this.alerts);
+    this.state = this.state.filter(item => alert !== item);
+    this.trigger(this.state);
   },
 
   getState() {
-    return this.alerts;
+    return this.state;
   },
 };