ref(js): Use a Map in SelectedGroupStore (#37976)

static/app/stores/selectedGroupStore.tsx

@@ -8,7 +8,11 @@ interface InternalDefinition {
    * The last item to have been selected
    */
   lastSelected: string | null;
-  records: Record<string, boolean>;
+  /**
+   * Mapping of item ID -> if it is selected. This is a map to
+   * make it easier to track if everything has been selected or not.
+   */
+  records: Map<string, boolean>;
 }
 
 interface SelectedGroupStoreDefinition extends StoreDefinition, InternalDefinition {
@@ -20,104 +24,96 @@ interface SelectedGroupStoreDefinition extends StoreDefinition, InternalDefiniti
   init(): void;
   isSelected(itemId: string): boolean;
   multiSelected(): boolean;
-  numSelected(): number;
-  onGroupChange(itemIds: string[]): void;
+  onGroupChange(itemIds: Set<string>): void;
   prune(): void;
+  reset(): void;
   shiftToggleItems(itemId: string): void;
   toggleSelect(itemId: string): void;
   toggleSelectAll(): void;
 }
 
 const storeConfig: SelectedGroupStoreDefinition = {
-  records: {},
+  records: new Map(),
   lastSelected: null,
   unsubscribeListeners: [],
 
   init() {
-    this.records = {};
-    this.lastSelected = null;
+    this.reset();
 
     this.unsubscribeListeners.push(
       this.listenTo(GroupStore, this.onGroupChange, this.onGroupChange)
     );
   },
 
+  reset() {
+    this.records = new Map();
+    this.lastSelected = null;
+  },
+
   onGroupChange(itemIds) {
     this.prune();
-    this.add(itemIds);
+    this.add([...itemIds]);
     this.trigger();
   },
 
   add(ids) {
     const allSelected = this.allSelected();
-    ids.forEach(id => {
-      if (!this.records.hasOwnProperty(id)) {
-        this.records[id] = allSelected;
-      }
-    });
+
+    ids
+      .filter(id => !this.records.has(id))
+      .forEach(id => this.records.set(id, allSelected));
   },
 
   prune() {
     const existingIds = new Set(GroupStore.getAllItemIds());
     this.lastSelected = null;
 
-    // Remove ids that no longer exist
-    for (const itemId in this.records) {
-      if (!existingIds.has(itemId)) {
-        delete this.records[itemId];
-      }
-    }
+    // Remove everything that no longer exists
+    [...this.records.keys()]
+      .filter(id => !existingIds.has(id))
+      .forEach(id => this.records.delete(id));
   },
 
   allSelected() {
     const itemIds = this.getSelectedIds();
-    const numRecords = this.numSelected();
 
-    return itemIds.size > 0 && itemIds.size === numRecords;
+    return itemIds.size > 0 && itemIds.size === this.records.size;
   },
 
   numSelected() {
-    return Object.keys(this.records).length;
+    return this.getSelectedIds().size;
   },
 
   anySelected() {
-    const itemIds = this.getSelectedIds();
-    return itemIds.size > 0;
+    return this.getSelectedIds().size > 0;
   },
 
   multiSelected() {
-    const itemIds = this.getSelectedIds();
-    return itemIds.size > 1;
+    return this.getSelectedIds().size > 1;
   },
 
   getSelectedIds() {
-    const selected = new Set<string>();
-    for (const itemId in this.records) {
-      if (this.records[itemId]) {
-        selected.add(itemId);
-      }
-    }
-    return selected;
+    return new Set([...this.records.keys()].filter(id => this.records.get(id)));
   },
 
   isSelected(itemId) {
-    return this.records[itemId] === true;
+    return !!this.records.get(itemId);
   },
 
   deselectAll() {
-    for (const itemId in this.records) {
-      this.records[itemId] = false;
-    }
-    this.lastSelected = null;
+    this.records.forEach((_, id) => this.records.set(id, false));
     this.trigger();
   },
 
   toggleSelect(itemId) {
-    if (!this.records.hasOwnProperty(itemId)) {
+    if (!this.records.has(itemId)) {
       return;
     }
-    this.records[itemId] = !this.records[itemId];
-    if (this.records[itemId]) {
+
+    const newState = !this.records.get(itemId);
+    this.records.set(itemId, newState);
+
+    if (newState) {
       this.lastSelected = itemId;
     }
     this.trigger();
@@ -127,15 +123,12 @@ const storeConfig: SelectedGroupStoreDefinition = {
     const allSelected = !this.allSelected();
     this.lastSelected = null;
 
-    for (const itemId in this.records) {
-      this.records[itemId] = allSelected;
-    }
-
+    this.records.forEach((_, id) => this.records.set(id, allSelected));
     this.trigger();
   },
 
   shiftToggleItems(itemId) {
-    if (!this.records.hasOwnProperty(itemId)) {
+    if (!this.records.has(itemId)) {
       return;
     }
     if (!this.lastSelected) {
@@ -151,16 +144,16 @@ const storeConfig: SelectedGroupStoreDefinition = {
       return;
     }
 
-    const newValue = !this.records[itemId];
+    const newValue = !this.records.get(itemId);
     const selected =
       lastIdx < currentIdx
         ? ids.slice(lastIdx, currentIdx)
         : ids.slice(currentIdx, lastIdx);
-    [...selected, this.lastSelected, itemId].forEach(id => {
-      if (this.records.hasOwnProperty(id)) {
-        this.records[id] = newValue;
-      }
-    });
+
+    [...selected, this.lastSelected, itemId]
+      .filter(id => this.records.has(id))
+      .forEach(id => this.records.set(id, newValue));
+
     this.lastSelected = itemId;
     this.trigger();
   },

tests/js/spec/stores/selectedGroupStore.spec.jsx → tests/js/spec/stores/selectedGroupStore.spec.tsx

@@ -1,52 +1,67 @@
 import GroupStore from 'sentry/stores/groupStore';
 import SelectedGroupStore from 'sentry/stores/selectedGroupStore';
 
+function makeRecords(records: Record<string, boolean>) {
+  return new Map(Object.entries(records));
+}
+
 describe('SelectedGroupStore', function () {
-  let trigger;
+  let trigger: jest.SpyInstance;
 
   beforeEach(function () {
-    SelectedGroupStore.records = {};
+    SelectedGroupStore.records = new Map();
 
     trigger = jest.spyOn(SelectedGroupStore, 'trigger').mockImplementation(() => {});
   });
 
   afterEach(function () {
-    SelectedGroupStore.trigger.mockRestore();
+    trigger.mockRestore();
   });
 
   describe('prune()', function () {
     it('removes records no longer in the GroupStore', function () {
       jest.spyOn(GroupStore, 'getAllItemIds').mockImplementation(() => ['3']);
-      SelectedGroupStore.records = {1: true, 2: true, 3: true};
+      SelectedGroupStore.records = makeRecords({1: true, 2: true, 3: true});
       SelectedGroupStore.prune();
-      expect(SelectedGroupStore.records).toEqual({3: true});
+      expect([...SelectedGroupStore.records.entries()]).toEqual([['3', true]]);
     });
 
     it("doesn't have any effect when already in sync", function () {
       jest.spyOn(GroupStore, 'getAllItemIds').mockImplementation(() => ['1', '2', '3']);
-      SelectedGroupStore.records = {1: true, 2: true, 3: true};
+      SelectedGroupStore.records = makeRecords({1: true, 2: true, 3: true});
       SelectedGroupStore.prune();
-      expect(SelectedGroupStore.records).toEqual({1: true, 2: true, 3: true});
+      expect([...SelectedGroupStore.records.entries()]).toEqual([
+        ['1', true],
+        ['2', true],
+        ['3', true],
+      ]);
     });
   });
 
   describe('add()', function () {
     it("defaults value of new ids to 'allSelected()'", function () {
-      SelectedGroupStore.records = {1: true};
-      SelectedGroupStore.add([2]);
-      expect(SelectedGroupStore.records).toEqual({1: true, 2: true});
+      SelectedGroupStore.records = makeRecords({1: true});
+      SelectedGroupStore.add(['2']);
+      expect([...SelectedGroupStore.records.entries()]).toEqual([
+        ['1', true],
+        ['2', true],
+      ]);
     });
 
     it('does not update existing ids', function () {
-      SelectedGroupStore.records = {1: false, 2: true};
-      SelectedGroupStore.add([3]);
-      expect(SelectedGroupStore.records).toEqual({1: false, 2: true, 3: false});
+      SelectedGroupStore.records = makeRecords({1: false, 2: true});
+      SelectedGroupStore.add(['3']);
+      expect([...SelectedGroupStore.records.entries()]).toEqual([
+        ['1', false],
+        ['2', true],
+        ['3', false],
+      ]);
     });
   });
 
   describe('onGroupChange()', function () {
-    let prune;
-    let add;
+    let prune: jest.SpyInstance;
+    let add: jest.SpyInstance;
 
     beforeEach(function () {
       prune = jest.spyOn(SelectedGroupStore, 'prune');
@@ -56,34 +71,34 @@ describe('SelectedGroupStore', function () {
     afterEach(function () {});
 
     it('adds new ids', function () {
-      SelectedGroupStore.onGroupChange([]);
+      SelectedGroupStore.onGroupChange(new Set());
       expect(add).toHaveBeenCalled();
     });
 
     it('prunes stale records', function () {
-      SelectedGroupStore.onGroupChange([]);
+      SelectedGroupStore.onGroupChange(new Set());
       expect(prune).toHaveBeenCalled();
     });
 
     it('triggers an update', function () {
-      SelectedGroupStore.onGroupChange([]);
+      SelectedGroupStore.onGroupChange(new Set());
       expect(trigger).toHaveBeenCalled();
     });
   });
 
   describe('allSelected()', function () {
     it('returns true when all ids are selected', function () {
-      SelectedGroupStore.records = {1: true, 2: true};
+      SelectedGroupStore.records = makeRecords({1: true, 2: true});
       expect(SelectedGroupStore.allSelected()).toBe(true);
     });
 
     it('returns false when some ids are selected', function () {
-      SelectedGroupStore.records = {1: true, 2: false};
+      SelectedGroupStore.records = makeRecords({1: true, 2: false});
       expect(SelectedGroupStore.allSelected()).toBe(false);
     });
 
     it('returns false when no ids are selected', function () {
-      SelectedGroupStore.records = {1: false, 2: false};
+      SelectedGroupStore.records = makeRecords({1: false, 2: false});
       expect(SelectedGroupStore.allSelected()).toBe(false);
     });
 
@@ -94,36 +109,36 @@ describe('SelectedGroupStore', function () {
 
   describe('anySelected()', function () {
     it('returns true if any ids are selected', function () {
-      SelectedGroupStore.records = {1: true, 2: false};
+      SelectedGroupStore.records = makeRecords({1: true, 2: false});
       expect(SelectedGroupStore.anySelected()).toBe(true);
     });
 
     it('returns false when no ids are selected', function () {
-      SelectedGroupStore.records = {1: false, 2: false};
+      SelectedGroupStore.records = makeRecords({1: false, 2: false});
       expect(SelectedGroupStore.anySelected()).toBe(false);
     });
   });
 
   describe('multiSelected()', function () {
     it('returns true when multiple ids are selected', function () {
-      SelectedGroupStore.records = {1: true, 2: true, 3: false};
+      SelectedGroupStore.records = makeRecords({1: true, 2: true, 3: false});
       expect(SelectedGroupStore.multiSelected()).toBe(true);
     });
 
     it('returns false when a single id is selected', function () {
-      SelectedGroupStore.records = {1: true, 2: false};
+      SelectedGroupStore.records = makeRecords({1: true, 2: false});
       expect(SelectedGroupStore.multiSelected()).toBe(false);
     });
 
     it('returns false when no ids are selected', function () {
-      SelectedGroupStore.records = {1: false, 2: false};
+      SelectedGroupStore.records = makeRecords({1: false, 2: false});
       expect(SelectedGroupStore.multiSelected()).toBe(false);
     });
   });
 
   describe('getSelectedIds()', function () {
     it('returns selected ids', function () {
-      SelectedGroupStore.records = {1: true, 2: false, 3: true};
+      SelectedGroupStore.records = makeRecords({1: true, 2: false, 3: true});
       const ids = SelectedGroupStore.getSelectedIds();
 
       expect(ids.has('1')).toBe(true);
@@ -132,7 +147,7 @@ describe('SelectedGroupStore', function () {
     });
 
     it('returns empty set with no selected ids', function () {
-      SelectedGroupStore.records = {1: false};
+      SelectedGroupStore.records = makeRecords({1: false});
       const ids = SelectedGroupStore.getSelectedIds();
 
       expect(ids.has('1')).toBe(false);
@@ -142,23 +157,27 @@ describe('SelectedGroupStore', function () {
 
   describe('isSelected()', function () {
     it('returns true if id is selected', function () {
-      SelectedGroupStore.records = {1: true};
-      expect(SelectedGroupStore.isSelected(1)).toBe(true);
+      SelectedGroupStore.records = makeRecords({1: true});
+      expect(SelectedGroupStore.isSelected('1')).toBe(true);
     });
 
     it('returns false if id is unselected or unknown', function () {
-      SelectedGroupStore.records = {1: false};
-      expect(SelectedGroupStore.isSelected(1)).toBe(false);
-      expect(SelectedGroupStore.isSelected(2)).toBe(false);
-      expect(SelectedGroupStore.isSelected()).toBe(false);
+      SelectedGroupStore.records = makeRecords({1: false});
+      expect(SelectedGroupStore.isSelected('1')).toBe(false);
+      expect(SelectedGroupStore.isSelected('2')).toBe(false);
+      expect(SelectedGroupStore.isSelected('')).toBe(false);
     });
   });
 
   describe('deselectAll()', function () {
     it('sets all records to false', function () {
-      SelectedGroupStore.records = {1: true, 2: true, 3: false};
+      SelectedGroupStore.records = makeRecords({1: true, 2: true, 3: false});
       SelectedGroupStore.deselectAll();
-      expect(SelectedGroupStore.records).toEqual({1: false, 2: false, 3: false});
+      expect([...SelectedGroupStore.records.entries()]).toEqual([
+        ['1', false],
+        ['2', false],
+        ['3', false],
+      ]);
     });
 
     it('triggers an update', function () {
@@ -169,41 +188,46 @@ describe('SelectedGroupStore', function () {
 
   describe('toggleSelect()', function () {
     it('toggles state given pre-existing id', function () {
-      SelectedGroupStore.records = {1: true};
-      SelectedGroupStore.toggleSelect(1);
-      expect(SelectedGroupStore.records[1]).toBe(false);
+      SelectedGroupStore.records = makeRecords({1: true});
+      SelectedGroupStore.toggleSelect('1');
+      expect(SelectedGroupStore.records.get('1')).toBe(false);
     });
 
     it('does not toggle state given unknown id', function () {
-      SelectedGroupStore.toggleSelect(1);
-      SelectedGroupStore.toggleSelect();
-      SelectedGroupStore.toggleSelect(undefined);
-      expect(SelectedGroupStore.records).toEqual({});
+      SelectedGroupStore.toggleSelect('1');
+      SelectedGroupStore.toggleSelect('');
+      expect([...SelectedGroupStore.records.entries()]).toEqual([]);
     });
 
     it('triggers an update given pre-existing id', function () {
-      SelectedGroupStore.records = {1: true};
-      SelectedGroupStore.toggleSelect(1);
+      SelectedGroupStore.records = makeRecords({1: true});
+      SelectedGroupStore.toggleSelect('1');
       expect(trigger).toHaveBeenCalled();
     });
 
     it('does not trigger an update given unknown id', function () {
-      SelectedGroupStore.toggleSelect();
+      SelectedGroupStore.toggleSelect('');
       expect(trigger).not.toHaveBeenCalled();
     });
   });
 
   describe('toggleSelectAll()', function () {
     it('selects all ids if any are unselected', function () {
-      SelectedGroupStore.records = {1: true, 2: false};
+      SelectedGroupStore.records = makeRecords({1: true, 2: false});
       SelectedGroupStore.toggleSelectAll();
-      expect(SelectedGroupStore.records).toEqual({1: true, 2: true});
+      expect([...SelectedGroupStore.records.entries()]).toEqual([
+        ['1', true],
+        ['2', true],
+      ]);
     });
 
     it('unselects all ids if all are selected', function () {
-      SelectedGroupStore.records = {1: true, 2: true};
+      SelectedGroupStore.records = makeRecords({1: true, 2: true});
       SelectedGroupStore.toggleSelectAll();
-      expect(SelectedGroupStore.records).toEqual({1: false, 2: false});
+      expect([...SelectedGroupStore.records.entries()]).toEqual([
+        ['1', false],
+        ['2', false],
+      ]);
     });
 
     it('triggers an update', function () {
@@ -219,13 +243,13 @@ describe('SelectedGroupStore', function () {
       SelectedGroupStore.add(ids);
       SelectedGroupStore.toggleSelect('12');
       SelectedGroupStore.shiftToggleItems('14');
-      expect(SelectedGroupStore.records).toEqual({
-        11: false,
-        12: true,
-        13: true,
-        14: true,
-        15: false,
-      });
+      expect([...SelectedGroupStore.records.entries()]).toEqual([
+        ['11', false],
+        ['12', true],
+        ['13', true],
+        ['14', true],
+        ['15', false],
+      ]);
     });
     it('toggles all between last selected and new selection backwards', function () {
       const ids = ['11', '12', '13', '14', '15'];
@@ -233,13 +257,13 @@ describe('SelectedGroupStore', function () {
       SelectedGroupStore.add(ids);
       SelectedGroupStore.toggleSelect('14');
       SelectedGroupStore.shiftToggleItems('12');
-      expect(SelectedGroupStore.records).toEqual({
-        11: false,
-        12: true,
-        13: true,
-        14: true,
-        15: false,
-      });
+      expect([...SelectedGroupStore.records.entries()]).toEqual([
+        ['11', false],
+        ['12', true],
+        ['13', true],
+        ['14', true],
+        ['15', false],
+      ]);
     });
     it('deslects after selecting', function () {
       const ids = ['11', '12', '13', '14', '15'];
@@ -249,23 +273,23 @@ describe('SelectedGroupStore', function () {
 
       // Select everything
       SelectedGroupStore.shiftToggleItems('15');
-      expect(SelectedGroupStore.records).toEqual({
-        11: true,
-        12: true,
-        13: true,
-        14: true,
-        15: true,
-      });
+      expect([...SelectedGroupStore.records.entries()]).toEqual([
+        ['11', true],
+        ['12', true],
+        ['13', true],
+        ['14', true],
+        ['15', true],
+      ]);
 
       // Deslect between selection (15) and 13
       SelectedGroupStore.shiftToggleItems('13');
-      expect(SelectedGroupStore.records).toEqual({
-        11: true,
-        12: true,
-        13: false,
-        14: false,
-        15: false,
-      });
+      expect([...SelectedGroupStore.records.entries()]).toEqual([
+        ['11', true],
+        ['12', true],
+        ['13', false],
+        ['14', false],
+        ['15', false],
+      ]);
     });
   });
 });

tests/js/spec/views/issueList/actions.spec.jsx

@@ -25,7 +25,7 @@ describe('IssueListActions', function () {
       beforeAll(function () {
         const {routerContext, org} = initializeOrg();
 
-        SelectedGroupStore.records = {};
+        SelectedGroupStore.reset();
         SelectedGroupStore.add(['1', '2', '3']);
         wrapper = mountWithTheme(
           <IssueListActions
@@ -94,7 +94,7 @@ describe('IssueListActions', function () {
 
     describe('Total results less than bulk limit', function () {
       beforeAll(function () {
-        SelectedGroupStore.records = {};
+        SelectedGroupStore.reset();
         SelectedGroupStore.add(['1', '2', '3']);
         wrapper = mountWithTheme(
           <IssueListActions
@@ -161,7 +161,7 @@ describe('IssueListActions', function () {
 
     describe('Selected on page', function () {
       beforeAll(function () {
-        SelectedGroupStore.records = {};
+        SelectedGroupStore.reset();
         SelectedGroupStore.add(['1', '2', '3']);
         wrapper = mountWithTheme(
           <IssueListActions
@@ -375,7 +375,7 @@ describe('IssueListActions', function () {
   describe('mark reviewed', function () {
     let issuesApiMock;
     beforeEach(() => {
-      SelectedGroupStore.records = {};
+      SelectedGroupStore.reset();
       const organization = TestStubs.Organization();
 
       wrapper = mountWithTheme(