Return a Map from groupBy/keyBy

I'd like to remove the `__proto__: empty` hacks which are buggy (see
Flow issue 8685).  We're currently returning objects with null
prototypes from groupBy/keyBy, because these objects are meant to be
used as maps.  So, we should really just be using maps.  I made that
change, which actually simplifies some of the usages.

Author: mwiencek

root/components/list/ReleaseGroupList.js

@@ -153,31 +153,30 @@ const ReleaseGroupList = ({
   sortable,
 }: ReleaseGroupListProps): Array<React$Node> => {
   const groupedReleaseGroups = groupBy(releaseGroups, x => x.typeName ?? '');
-  return (
-    Object.keys(groupedReleaseGroups).map<React$Node>((type) => {
-      const releaseGroupsOfType = groupedReleaseGroups[type];
-      return (
-        <React.Fragment key={type}>
-          <h3>
-            {type === ''
-              ? l('Unspecified type')
-              : releaseGroupType(releaseGroupsOfType[0])
-            }
-          </h3>
-          <ReleaseGroupListTable
-            checkboxes={checkboxes}
-            mergeForm={mergeForm}
-            order={order}
-            releaseGroups={releaseGroupsOfType}
-            seriesItemNumbers={seriesItemNumbers}
-            showRatings={showRatings}
-            showType={false}
-            sortable={sortable}
-          />
-        </React.Fragment>
-      );
-    })
-  );
+  const tables = [];
+  for (const [type, releaseGroupsOfType] of groupedReleaseGroups) {
+    tables.push(
+      <React.Fragment key={type}>
+        <h3>
+          {type === ''
+            ? l('Unspecified type')
+            : releaseGroupType(releaseGroupsOfType[0])
+          }
+        </h3>
+        <ReleaseGroupListTable
+          checkboxes={checkboxes}
+          mergeForm={mergeForm}
+          order={order}
+          releaseGroups={releaseGroupsOfType}
+          seriesItemNumbers={seriesItemNumbers}
+          showRatings={showRatings}
+          showType={false}
+          sortable={sortable}
+        />
+      </React.Fragment>,
+    );
+  }
+  return tables;
 };
 
 export default ReleaseGroupList;

root/static/scripts/common/MB/Control/Autocomplete.js

@@ -763,7 +763,7 @@ MB.Control.autocomplete_formatters = {
     if (item.labels) {
       for (
         const [name, releaseLabels] of
-        Object.entries(groupBy(item.labels, getLabelName))
+        groupBy(item.labels, getLabelName)
       ) {
         const catalogNumbers =
           compactMap(releaseLabels, getCatalogNumber)

root/static/scripts/common/components/TagEditor.js

@@ -685,7 +685,7 @@ function createInitialTagState(
   const used = new Set();
 
   const combined = aggregatedTags.map(function (t) {
-    const userTag = userTagsByName[t.tag.name];
+    const userTag = userTagsByName.get(t.tag.name);
 
     used.add(t.tag.name);
 
@@ -697,8 +697,7 @@ function createInitialTagState(
   });
 
   // Always show upvoted user tags (affects sidebar)
-  for (const tagName of Object.keys(userTagsByName)) {
-    const tag = userTagsByName[tagName];
+  for (const [tagName, tag] of userTagsByName) {
     if (tag.vote > 0 && !used.has(tagName)) {
       combined.push(tag);
     }

root/static/scripts/common/utility/arrays.js

@@ -130,32 +130,34 @@ export function sortByString<T>(
   return keys.map(x => array[x[0]]);
 }
 
-export function groupBy<T>(
+export function groupBy<T, K>(
   array: $ReadOnlyArray<T>,
-  func: (T) => string,
-): {__proto__: empty, [groupKey: string]: $ReadOnlyArray<T>} {
+  func: (T) => K,
+): Map<K, Array<T>> {
   return array.reduce(function (result, item) {
     const key = func(item);
-    if (!(key in result)) {
-      result[key] = [];
+    let values = result.get(key);
+    if (values == null) {
+      values = [];
+      result.set(key, values);
     }
-    result[key].push(item);
+    values.push(item);
     return result;
-  }, Object.create(null));
+  }, new Map());
 }
 
 export function first<T>(array: ?$ReadOnlyArray<T>): ?T {
   return array?.length ? array[0] : undefined;
 }
 
-export function keyBy<T>(
+export function keyBy<T, K = string>(
   array: $ReadOnlyArray<T>,
-  func: (T) => string,
-): {__proto__: empty, [groupKey: string]: T} {
+  func: (T) => K,
+): Map<K, T> {
   return array.reduce(function (result, item) {
-    result[func(item)] = item;
+    result.set(func(item), item);
     return result;
-  }, Object.create(null));
+  }, new Map());
 }
 
 export function last<T>(array: ?$ReadOnlyArray<T>): ?T {

root/static/scripts/edit/components/edit/RelationshipDiff.js

@@ -58,11 +58,11 @@ const RelationshipDiff = ({
       const typeId = String(attr.typeID);
       const display = displayLinkAttribute(attr);
 
-      if (oldAttrs[typeId] && !newAttrs[typeId]) {
+      if (oldAttrs.has(typeId) && !newAttrs.get(typeId)) {
         return diffOnlyA(display);
       }
 
-      if (newAttrs[typeId] && !oldAttrs[typeId]) {
+      if (newAttrs.get(typeId) && !oldAttrs.has(typeId)) {
         return diffOnlyB(display);
       }
 

root/static/scripts/edit/components/edit/ReleaseEventsDiff.js

@@ -81,37 +81,37 @@ const ReleaseEventsDiff = ({
   const oldEventsByCountry = keyBy(oldEvents, getCountryId);
   const newEventsByCountry = keyBy(newEvents, getCountryId);
 
-  const oldKeys = Object.keys(oldEventsByCountry).sort();
-  const newKeys = Object.keys(newEventsByCountry).sort();
+  const oldKeys = Array.from(oldEventsByCountry.keys()).sort();
+  const newKeys = Array.from(newEventsByCountry.keys()).sort();
 
   const oldSide = [];
   const newSide = [];
 
   for (let i = 0; i < oldKeys.length; i++) {
     const key = oldKeys[i];
-    const oldEvent = oldEventsByCountry[key];
-    let newEvent = newEventsByCountry[key];
+    const oldEvent = oldEventsByCountry.get(key);
+    let newEvent = newEventsByCountry.get(key);
     /*
      * If this country was removed, compare against the new entry at
      * the same position visually.
      */
     if (!newEvent && i < newKeys.length) {
-      newEvent = newEventsByCountry[newKeys[i]];
+      newEvent = newEventsByCountry.get(newKeys[i]);
     }
     oldSide.push(changeSide(oldEvent, newEvent, DELETE));
   }
 
   for (let i = 0; i < newKeys.length; i++) {
     const key = newKeys[i];
-    let oldEvent = oldEventsByCountry[key];
+    let oldEvent = oldEventsByCountry.get(key);
     /*
      * If this country was added, compare against the old entry at
      * the same position visually.
      */
     if (!oldEvent && i < oldKeys.length) {
-      oldEvent = oldEventsByCountry[oldKeys[i]];
+      oldEvent = oldEventsByCountry.get(oldKeys[i]);
     }
-    const newEvent = newEventsByCountry[key];
+    const newEvent = newEventsByCountry.get(key);
     newSide.push(changeSide(oldEvent, newEvent, INSERT));
   }
 

root/static/scripts/edit/externalLinks.js

@@ -36,18 +36,15 @@ import * as URLCleanup from './URLCleanup';
 import validation from './validation';
 
 type LinkStateT = {
-  relationship: number | string | null,
+  // New relationships will use a unique string ID like "new-1".
+  relationship: StrOrNum | null,
   type: number | null,
   url: string,
   video: boolean,
   ...
 };
 
-type LinkHashT = {
-  __proto__: empty,
-  +[key: number | string | null]: LinkStateT,
-  ...
-};
+type LinkMapT = Map<string, LinkStateT>;
 
 type LinksEditorProps = {
   errorObservable: (boolean) => void,
@@ -135,27 +132,27 @@ export class ExternalLinksEditor
     });
   }
 
-  getOldLinksHash(): LinkHashT {
-    return keyBy(
+  getOldLinksHash(): LinkMapT {
+    return keyBy<LinkStateT, string>(
       this.props.initialLinks
         .filter(link => isPositiveInteger(link.relationship)),
       x => String(x.relationship),
     );
   }
 
   getEditData(): {
-    allLinks: LinkHashT,
-    newLinks: LinkHashT,
-    oldLinks: LinkHashT,
+    allLinks: LinkMapT,
+    newLinks: LinkMapT,
+    oldLinks: LinkMapT,
     } {
     const oldLinks = this.getOldLinksHash();
-    const newLinks = keyBy<
-      LinkStateT,
-      $ElementType<LinkStateT, 'relationship'>,
-    >(this.state.links, x => String(x.relationship));
+    const newLinks: LinkMapT = keyBy(
+      this.state.links,
+      x => String(x.relationship),
+    );
 
     return {
-      allLinks: {...oldLinks, ...newLinks},
+      allLinks: new Map([...oldLinks, ...newLinks]),
       newLinks: newLinks,
       oldLinks: oldLinks,
     };
@@ -170,10 +167,7 @@ export class ExternalLinksEditor
     const backward = this.props.sourceType > 'url';
     const {oldLinks, newLinks, allLinks} = this.getEditData();
 
-    for (
-      const [relationship, link] of
-      ((Object.entries(allLinks): any): $ReadOnlyArray<[string, ?LinkStateT]>)
-    ) {
+    for (const [relationship, link] of allLinks) {
       if (!link?.type) {
         return;
       }
@@ -183,7 +177,7 @@ export class ExternalLinksEditor
       if (isPositiveInteger(relationship)) {
         pushInput(prefix, 'relationship_id', String(relationship));
 
-        if (!newLinks[relationship]) {
+        if (!newLinks.has(relationship)) {
           pushInput(prefix, 'removed', '1');
         }
       }
@@ -192,7 +186,7 @@ export class ExternalLinksEditor
 
       if (link.video) {
         pushInput(prefix + '.attributes.0', 'type.gid', VIDEO_ATTRIBUTE_GID);
-      } else if ((oldLinks[relationship] || {}).video) {
+      } else if (oldLinks.get(relationship)?.video) {
         pushInput(prefix + '.attributes.0', 'type.gid', VIDEO_ATTRIBUTE_GID);
         pushInput(prefix + '.attributes.0', 'removed', '1');
       }
@@ -231,7 +225,7 @@ export class ExternalLinksEditor
             const linkType = link.type
               ? linkedEntities.link_type[link.type] : {};
             const checker = URLCleanup.validationRules[linkType.gid];
-            const oldLink = oldLinks[link.relationship];
+            const oldLink = oldLinks.get(String(link.relationship));
             const isNewLink = !isPositiveInteger(link.relationship);
             const linkChanged = oldLink && link.url !== oldLink.url;
             const isNewOrChangedLink = (isNewLink || linkChanged);
@@ -274,7 +268,9 @@ export class ExternalLinksEditor
                          and should not be used.`);
               errorTarget = URLCleanup.ERROR_TARGETS.RELATIONSHIP;
             } else if (
-              (linksByTypeAndUrl[linkTypeAndUrlString(link)] || []).length > 1
+              (linksByTypeAndUrl.get(
+                linkTypeAndUrlString(link),
+              ) || []).length > 1
             ) {
               error = l('This relationship already exists.');
               errorTarget = URLCleanup.ERROR_TARGETS.RELATIONSHIP;

root/static/scripts/relationship-editor/common/dialog.js

@@ -912,7 +912,7 @@ function splitByCreditableAttributes(relationship) {
 
   for (
     const attributes of
-    Object.values(groupBy(creditable, linkAttributeTypeID))
+    groupBy(creditable, linkAttributeTypeID).values()
   ) {
     const extra = attributes.slice(1);
     relationship.attributes.removeAll(extra);

root/static/scripts/relationship-editor/common/viewModel.js

@@ -56,7 +56,7 @@ RE.exportTypeInfo = function (typeInfo, attrInfo) {
     }
     switch (item.entityType) {
       case 'link_attribute_type':
-        const children = attrChildren[item.id];
+        const children = attrChildren.get(item.id);
         if (children) {
           item.children = children;
         }

root/static/scripts/release-editor/edits.js

@@ -114,10 +114,10 @@ releaseEditor.edits = {
     var edits = [];
 
     for (let newLabel of newLabels) {
-      const id = newLabel.release_label;
+      const id = getReleaseLabel(newLabel);
 
       if (id) {
-        const oldLabel = oldLabelsByID[id];
+        const oldLabel = oldLabelsByID.get(id);
 
         if (oldLabel && !deepEqual(newLabel, oldLabel)) {
           // Edit ReleaseLabel
@@ -135,8 +135,8 @@ releaseEditor.edits = {
     }
 
     for (let oldLabel of oldLabels) {
-      const id = oldLabel.release_label;
-      const newLabel = newLabelsByID[id];
+      const id = getReleaseLabel(oldLabel);
+      const newLabel = newLabelsByID.get(id);
 
       if (!newLabel || !(newLabel.label || newLabel.catalog_number)) {
         // Delete ReleaseLabel
@@ -383,19 +383,21 @@ releaseEditor.edits = {
       return edits;
     }
 
-    for (const link of Object.values(allLinks)) {
+    for (const link of allLinks.values()) {
       if (!link.type || !link.url) {
         continue;
       }
 
       const newData = MB.edit.fields.externalLinkRelationship(link, release);
+      const relationshipId = link.relationship;
+      const relationshipIdString = String(relationshipId);
 
       if (isPositiveInteger(link.relationship)) {
-        if (!newLinks[link.relationship]) {
+        if (!newLinks.has(relationshipIdString)) {
           edits.push(MB.edit.relationshipDelete(newData));
-        } else if (oldLinks[link.relationship]) {
+        } else if (oldLinks.has(relationshipIdString)) {
           const original = MB.edit.fields.externalLinkRelationship(
-            oldLinks[link.relationship],
+            oldLinks.get(relationshipIdString),
             release,
           );
 
@@ -412,7 +414,7 @@ releaseEditor.edits = {
             edits.push(editData);
           }
         }
-      } else if (newLinks[link.relationship]) {
+      } else if (newLinks.has(relationshipIdString)) {
         edits.push(MB.edit.relationshipCreate(newData));
       }
     }
@@ -667,7 +669,9 @@ releaseEditor.orderedEditSubmissions = [
       var newMediums = release.mediums();
 
       newMediums.filter(m => m.id == null).forEach(function (medium) {
-        var addedData = added[medium.tmpPosition || medium.position()];
+        var addedData = added.get(
+          String(medium.tmpPosition || medium.position()),
+        );
 
         if (addedData) {
           medium.id = addedData.id;