ref(OrganizationsStore): Return stable ref from getState (#71372)

static/app/stores/latestContextStore.tsx

@@ -1,6 +1,6 @@
-import type {StoreDefinition} from 'reflux';
 import {createStore} from 'reflux';
 
+import type {StrictStoreDefinition} from 'sentry/stores/types';
 import type {Organization} from 'sentry/types/organization';
 import type {Project} from 'sentry/types/project';
 
@@ -11,14 +11,13 @@ type State = {
   project: Project | null;
 };
 
-interface LatestContextStoreDefinition extends StoreDefinition {
+interface LatestContextStoreDefinition extends StrictStoreDefinition<State> {
   get(): State;
   onSetActiveOrganization(organization: Organization): void;
   onSetActiveProject(project: Project | null): void;
   onUpdateOrganization(organization: Partial<Organization>): void;
   onUpdateProject(project: Project | null): void;
   reset(): void;
-  state: State;
 }
 
 /**
@@ -123,6 +122,10 @@ const storeConfig: LatestContextStoreDefinition = {
     };
     this.trigger(this.state);
   },
+
+  getState() {
+    return this.state;
+  },
 };
 
 const LatestContextStore = createStore(storeConfig);

static/app/stores/memberListStore.tsx

@@ -1,6 +1,6 @@
-import type {StoreDefinition} from 'reflux';
 import {createStore} from 'reflux';
 
+import type {StrictStoreDefinition} from 'sentry/stores/types';
 import type {User} from 'sentry/types/user';
 
 type State = {
@@ -13,14 +13,12 @@ type State = {
 // XXX(epurkhiser): Either this store is completely wrong, or it is misnamed, a
 // `Member` has one `User`, this stores users not members.
 
-interface MemberListStoreDefinition extends StoreDefinition {
+interface MemberListStoreDefinition extends StrictStoreDefinition<State> {
   getAll(): User[];
   getById(memberId: string): User | undefined;
-  getState(): State;
   init(): void;
   loadInitialData(items: User[], hasMore?: boolean | null, cursor?: string | null): void;
   reset(): void;
-  state: State;
 }
 
 const storeConfig: MemberListStoreDefinition = {

static/app/stores/modalStore.tsx

@@ -2,7 +2,7 @@ import {createStore} from 'reflux';
 
 import type {ModalOptions, ModalRenderProps} from 'sentry/actionCreators/modal';
 
-import type {CommonStoreDefinition} from './types';
+import type {StrictStoreDefinition} from './types';
 
 type Renderer = (renderProps: ModalRenderProps) => React.ReactNode;
 
@@ -11,15 +11,15 @@ type State = {
   renderer: Renderer | null;
 };
 
-interface ModalStoreDefinition extends CommonStoreDefinition<State> {
+interface ModalStoreDefinition extends StrictStoreDefinition<State> {
   closeModal(): void;
-  getState(): State;
   init(): void;
   openModal(renderer: Renderer, options: ModalOptions): void;
   reset(): void;
 }
 
 const storeConfig: ModalStoreDefinition = {
+  state: {renderer: null, options: {}},
   init() {
     // XXX: Do not use `this.listenTo` in this store. We avoid usage of reflux
     // listeners due to their leaky nature in tests.
@@ -35,7 +35,7 @@ const storeConfig: ModalStoreDefinition = {
     this.state = {
       renderer: null,
       options: {},
-    } as State;
+    };
   },
 
   closeModal() {

static/app/stores/organizationsStore.spec.tsx

@@ -0,0 +1,65 @@
+import {OrganizationFixture} from 'sentry-fixture/organization';
+
+import OrganizationsStore from 'sentry/stores/organizationsStore';
+
+describe('OrganizationsStore', () => {
+  beforeEach(() => {
+    OrganizationsStore.init();
+  });
+
+  it('starts with loading state', () => {
+    expect(OrganizationsStore.getState()).toEqual({
+      organizations: [],
+      loaded: false,
+    });
+  });
+
+  it('updates slug correctly', () => {
+    const organization = OrganizationFixture();
+    OrganizationsStore.load([organization]);
+    expect(OrganizationsStore.getState()).toEqual({
+      organizations: [organization],
+      loaded: true,
+    });
+
+    const update = {...organization, slug: 'california'};
+    OrganizationsStore.onChangeSlug(organization, update);
+    expect(OrganizationsStore.getState()).toMatchObject({
+      organizations: [update],
+      loaded: true,
+    });
+  });
+
+  it('updates property correctly', () => {
+    const organization = OrganizationFixture();
+    OrganizationsStore.load([organization]);
+    expect(OrganizationsStore.getState()).toEqual({
+      organizations: [organization],
+      loaded: true,
+    });
+
+    const update = {...organization, something: true};
+    OrganizationsStore.onUpdate(update);
+    expect(OrganizationsStore.getState()).toMatchObject({
+      organizations: [update],
+      loaded: true,
+    });
+  });
+
+  it('adds an organization', () => {
+    const organization = OrganizationFixture();
+    OrganizationsStore.load([organization]);
+
+    const newOrg = OrganizationFixture({id: '2', slug: 'new'});
+    OrganizationsStore.addOrReplace(newOrg);
+    expect(OrganizationsStore.getState()).toMatchObject({
+      organizations: [organization, newOrg],
+      loaded: true,
+    });
+  });
+
+  it('returns a stable reference with getState', () => {
+    const state = OrganizationsStore.getState();
+    expect(Object.is(state, OrganizationsStore.getState())).toBe(true);
+  });
+});

static/app/stores/organizationsStore.tsx

@@ -2,19 +2,17 @@ import {createStore} from 'reflux';
 
 import type {Organization} from 'sentry/types/organization';
 
-import type {CommonStoreDefinition} from './types';
+import type {StrictStoreDefinition} from './types';
 
 interface State {
   loaded: boolean;
   organizations: Organization[];
 }
 
-interface OrganizationsStoreDefinition extends State, CommonStoreDefinition<State> {
+interface OrganizationsStoreDefinition extends StrictStoreDefinition<State> {
   addOrReplace(item: Organization): void;
   get(slug: string): Organization | undefined;
-
   getAll(): Organization[];
-  init(): void;
   load(items: Organization[]): void;
   onChangeSlug(prev: Organization, next: Partial<Organization>): void;
   onRemoveSuccess(slug: string): void;
@@ -23,27 +21,21 @@ interface OrganizationsStoreDefinition extends State, CommonStoreDefinition<Stat
 }
 
 const storeConfig: OrganizationsStoreDefinition = {
-  organizations: [],
-  loaded: false,
-
-  // So we can use Reflux.connect in a component mixin
-  getInitialState() {
-    return this.organizations;
-  },
+  state: {organizations: [], loaded: false},
 
   init() {
     // XXX: Do not use `this.listenTo` in this store. We avoid usage of reflux
     // listeners due to their leaky nature in tests.
 
-    this.organizations = [];
-    this.loaded = false;
+    this.state = {organizations: [], loaded: false};
   },
 
   onUpdate(org) {
     let match = false;
-    this.organizations.forEach((existing, idx) => {
+    const newOrgs = [...this.state.organizations];
+    newOrgs.forEach((existing, idx) => {
       if (existing.id === org.id) {
-        this.organizations[idx] = {...existing, ...org};
+        newOrgs[idx] = {...existing, ...org};
         match = true;
       }
     });
@@ -52,7 +44,8 @@ const storeConfig: OrganizationsStoreDefinition = {
         'Cannot update an organization that is not in the OrganizationsStore'
       );
     }
-    this.trigger(this.organizations);
+    this.state = {...this.state, organizations: newOrgs};
+    this.trigger(newOrgs);
   },
 
   onChangeSlug(prev, next) {
@@ -69,40 +62,45 @@ const storeConfig: OrganizationsStoreDefinition = {
   },
 
   get(slug) {
-    return this.organizations.find((item: Organization) => item.slug === slug);
+    return this.state.organizations.find((item: Organization) => item.slug === slug);
   },
 
   getAll() {
-    return this.organizations;
+    return this.state.organizations;
   },
 
   getState() {
-    return {organizations: this.organizations, loaded: this.loaded};
+    return this.state;
   },
 
   remove(slug) {
-    this.organizations = this.organizations.filter(item => slug !== item.slug);
-    this.trigger(this.organizations);
+    this.state = {
+      ...this.state,
+      organizations: this.state.organizations.filter(item => slug !== item.slug),
+    };
+    this.trigger(this.state.organizations);
   },
 
   addOrReplace(item) {
     let match = false;
-    this.organizations.forEach((existing, idx) => {
+    const newOrgs = [...this.state.organizations];
+    newOrgs.forEach((existing, idx) => {
       if (existing.id === item.id) {
-        this.organizations[idx] = {...existing, ...item};
+        newOrgs[idx] = {...existing, ...item};
         match = true;
       }
     });
     if (!match) {
-      this.organizations = [...this.organizations, item];
+      newOrgs.push(item);
     }
-    this.trigger(this.organizations);
+    this.state = {...this.state, organizations: newOrgs};
+    this.trigger(newOrgs);
   },
 
   load(items: Organization[]) {
-    this.organizations = items;
-    this.loaded = true;
-    this.trigger(items);
+    const newOrgs = [...items];
+    this.state = {organizations: newOrgs, loaded: true};
+    this.trigger(newOrgs);
   },
 };
 

static/app/stores/types.tsx

@@ -3,6 +3,8 @@
  * the stores state.
  *
  * When a store implements this it becomes usable with the `useLegacyStore` hook.
+ *
+ * @deprecated Use `StrictStoreDefinition` instead
  */
 export interface CommonStoreDefinition<T> extends Reflux.StoreDefinition {
   /**
@@ -10,3 +12,20 @@ export interface CommonStoreDefinition<T> extends Reflux.StoreDefinition {
    */
   getState(): T;
 }
+
+/**
+ * Does not have the `[key: string]: any;` index signature that `Reflux.StoreDefinition` has.
+ */
+export interface StrictStoreDefinition<T> {
+  /**
+   * Returns the current state represented within the store
+   */
+  getState(): Readonly<T>;
+  init(): void;
+  state: Readonly<T>;
+  /**
+   * Trigger is not defined by the store definition, but is added by Reflux
+   * We could try to type this better, but would need to update all .listen calls
+   */
+  trigger?: any;
+}

static/app/utils/withOrganizations.tsx

@@ -1,7 +1,7 @@
 import {Component} from 'react';
 
 import OrganizationsStore from 'sentry/stores/organizationsStore';
-import type {OrganizationSummary} from 'sentry/types';
+import type {OrganizationSummary} from 'sentry/types/organization';
 import getDisplayName from 'sentry/utils/getDisplayName';
 
 type InjectedOrganizationsProps = {
@@ -38,7 +38,8 @@ function withOrganizations<P extends InjectedOrganizationsProps>(
       return (
         <WrappedComponent
           {...({
-            organizationsLoading: organizationsLoading ?? !OrganizationsStore.loaded,
+            organizationsLoading:
+              organizationsLoading ?? !OrganizationsStore.getState().loaded,
             organizations: organizations ?? this.state.organizations,
             ...props,
           } as P)}