remove closures from mutateElement, get rid of the element spreading (#902)

src/actions/actionFinalize.tsx

@@ -19,11 +19,11 @@ export const actionFinalize = register({
     if (appState.multiElement) {
       // pen and mouse have hover
       if (appState.lastPointerDownWith !== "touch") {
-        mutateElement(appState.multiElement, multiElement => {
-          multiElement.points = multiElement.points.slice(
+        mutateElement(appState.multiElement, {
+          points: appState.multiElement.points.slice(
             0,
-            multiElement.points.length - 1,
-          );
+            appState.multiElement.points.length - 1,
+          ),
         });
       }
       if (isInvisiblySmallElement(appState.multiElement)) {

src/actions/actionProperties.tsx

@@ -11,6 +11,7 @@ import { AppState } from "../../src/types";
 import { t } from "../i18n";
 import { DEFAULT_FONT } from "../appState";
 import { register } from "./register";
+import { newElementWith, newTextElementWith } from "../element/mutateElement";
 
 const changeProperty = (
   elements: readonly ExcalidrawElement[],
@@ -45,10 +46,11 @@ export const actionChangeStrokeColor = register({
   name: "changeStrokeColor",
   perform: (elements, appState, value) => {
     return {
-      elements: changeProperty(elements, appState, el => ({
-        ...el,
-        strokeColor: value,
-      })),
+      elements: changeProperty(elements, appState, el =>
+        newElementWith(el, {
+          strokeColor: value,
+        }),
+      ),
       appState: { ...appState, currentItemStrokeColor: value },
     };
   },
@@ -75,10 +77,11 @@ export const actionChangeBackgroundColor = register({
   name: "changeBackgroundColor",
   perform: (elements, appState, value) => {
     return {
-      elements: changeProperty(elements, appState, el => ({
-        ...el,
-        backgroundColor: value,
-      })),
+      elements: changeProperty(elements, appState, el =>
+        newElementWith(el, {
+          backgroundColor: value,
+        }),
+      ),
       appState: { ...appState, currentItemBackgroundColor: value },
     };
   },
@@ -105,10 +108,11 @@ export const actionChangeFillStyle = register({
   name: "changeFillStyle",
   perform: (elements, appState, value) => {
     return {
-      elements: changeProperty(elements, appState, el => ({
-        ...el,
-        fillStyle: value,
-      })),
+      elements: changeProperty(elements, appState, el =>
+        newElementWith(el, {
+          fillStyle: value,
+        }),
+      ),
       appState: { ...appState, currentItemFillStyle: value },
     };
   },
@@ -141,10 +145,11 @@ export const actionChangeStrokeWidth = register({
   name: "changeStrokeWidth",
   perform: (elements, appState, value) => {
     return {
-      elements: changeProperty(elements, appState, el => ({
-        ...el,
-        strokeWidth: value,
-      })),
+      elements: changeProperty(elements, appState, el =>
+        newElementWith(el, {
+          strokeWidth: value,
+        }),
+      ),
       appState: { ...appState, currentItemStrokeWidth: value },
     };
   },
@@ -175,10 +180,11 @@ export const actionChangeSloppiness = register({
   name: "changeSloppiness",
   perform: (elements, appState, value) => {
     return {
-      elements: changeProperty(elements, appState, el => ({
-        ...el,
-        roughness: value,
-      })),
+      elements: changeProperty(elements, appState, el =>
+        newElementWith(el, {
+          roughness: value,
+        }),
+      ),
       appState: { ...appState, currentItemRoughness: value },
     };
   },
@@ -209,10 +215,11 @@ export const actionChangeOpacity = register({
   name: "changeOpacity",
   perform: (elements, appState, value) => {
     return {
-      elements: changeProperty(elements, appState, el => ({
-        ...el,
-        opacity: value,
-      })),
+      elements: changeProperty(elements, appState, el =>
+        newElementWith(el, {
+          opacity: value,
+        }),
+      ),
       appState: { ...appState, currentItemOpacity: value },
     };
   },
@@ -259,10 +266,9 @@ export const actionChangeFontSize = register({
     return {
       elements: changeProperty(elements, appState, el => {
         if (isTextElement(el)) {
-          const element: ExcalidrawTextElement = {
-            ...el,
+          const element: ExcalidrawTextElement = newTextElementWith(el, {
             font: `${value}px ${el.font.split("px ")[1]}`,
-          };
+          });
           redrawTextBoundingBox(element);
           return element;
         }
@@ -307,10 +313,9 @@ export const actionChangeFontFamily = register({
     return {
       elements: changeProperty(elements, appState, el => {
         if (isTextElement(el)) {
-          const element: ExcalidrawTextElement = {
-            ...el,
+          const element: ExcalidrawTextElement = newTextElementWith(el, {
             font: `${el.font.split("px ")[0]}px ${value}`,
-          };
+          });
           redrawTextBoundingBox(element);
           return element;
         }

src/actions/actionStyles.ts

@@ -6,7 +6,7 @@ import {
 import { KEYS } from "../keys";
 import { DEFAULT_FONT } from "../appState";
 import { register } from "./register";
-import { mutateTextElement } from "../element/mutateElement";
+import { mutateTextElement, newElementWith } from "../element/mutateElement";
 
 let copiedStyles: string = "{}";
 
@@ -35,20 +35,19 @@ export const actionPasteStyles = register({
     return {
       elements: elements.map(element => {
         if (appState.selectedElementIds[element.id]) {
-          const newElement = {
-            ...element,
+          const newElement = newElementWith(element, {
             backgroundColor: pastedElement?.backgroundColor,
             strokeWidth: pastedElement?.strokeWidth,
             strokeColor: pastedElement?.strokeColor,
             fillStyle: pastedElement?.fillStyle,
             opacity: pastedElement?.opacity,
             roughness: pastedElement?.roughness,
-          };
+          });
           if (isTextElement(newElement)) {
-            mutateTextElement(newElement, newElement => {
-              newElement.font = pastedElement?.font || DEFAULT_FONT;
-              redrawTextBoundingBox(newElement);
+            mutateTextElement(newElement, {
+              font: pastedElement?.font || DEFAULT_FONT,
             });
+            redrawTextBoundingBox(newElement);
           }
           return newElement;
         }

src/components/App.tsx

@@ -88,7 +88,7 @@ import { LayerUI } from "./LayerUI";
 import { ScrollBars } from "../scene/types";
 import { invalidateShapeForElement } from "../renderer/renderElement";
 import { generateCollaborationLink, getCollaborationLinkData } from "../data";
-import { mutateElement } from "../element/mutateElement";
+import { mutateElement, newElementWith } from "../element/mutateElement";
 
 // -----------------------------------------------------------------------------
 // TEST HOOKS
@@ -473,17 +473,17 @@ export class App extends React.Component<any, AppState> {
         : ELEMENT_TRANSLATE_AMOUNT;
       elements = elements.map(el => {
         if (this.state.selectedElementIds[el.id]) {
-          const element = { ...el };
+          const update: { x?: number; y?: number } = {};
           if (event.key === KEYS.ARROW_LEFT) {
-            element.x -= step;
+            update.x = el.x - step;
           } else if (event.key === KEYS.ARROW_RIGHT) {
-            element.x += step;
+            update.x = el.x + step;
           } else if (event.key === KEYS.ARROW_UP) {
-            element.y -= step;
+            update.y = el.y - step;
           } else if (event.key === KEYS.ARROW_DOWN) {
-            element.y += step;
+            update.y = el.y + step;
           }
-          return element;
+          return newElementWith(el, update);
         }
         return el;
       });
@@ -803,9 +803,9 @@ export class App extends React.Component<any, AppState> {
       textY = centerElementYInViewport;
 
       // x and y will change after calling newTextElement function
-      mutateElement(element, element => {
-        element.x = centerElementX;
-        element.y = centerElementY;
+      mutateElement(element, {
+        x: centerElementX,
+        y: centerElementY,
       });
     } else if (!event.altKey) {
       const snappedToCenterPosition = this.getTextWysiwygSnappedToCenterPosition(
@@ -814,9 +814,9 @@ export class App extends React.Component<any, AppState> {
       );
 
       if (snappedToCenterPosition) {
-        mutateElement(element, element => {
-          element.x = snappedToCenterPosition.elementCenterX;
-          element.y = snappedToCenterPosition.elementCenterY;
+        mutateElement(element, {
+          x: snappedToCenterPosition.elementCenterX,
+          y: snappedToCenterPosition.elementCenterY,
         });
         textX = snappedToCenterPosition.wysiwygX;
         textY = snappedToCenterPosition.wysiwygY;
@@ -1369,17 +1369,18 @@ export class App extends React.Component<any, AppState> {
 
         const dx = element.x + width + p1[0];
         const dy = element.y + height + p1[1];
-        mutateElement(element, element => {
-          element.x = dx;
-          element.y = dy;
+        mutateElement(element, {
+          x: dx,
+          y: dy,
         });
         p1[0] = absPx - element.x;
         p1[1] = absPy - element.y;
       } else {
-        mutateElement(element, element => {
-          element.x += deltaX;
-          element.y += deltaY;
+        mutateElement(element, {
+          x: element.x + deltaX,
+          y: element.y + deltaY,
         });
+
         p1[0] -= deltaX;
         p1[1] -= deltaY;
       }
@@ -1489,16 +1490,15 @@ export class App extends React.Component<any, AppState> {
                   event.shiftKey,
                 );
               } else {
-                mutateElement(element, element => {
-                  element.width -= deltaX;
-                  element.x += deltaX;
-                  if (event.shiftKey) {
-                    element.y += element.height - element.width;
-                    element.height = element.width;
-                  } else {
-                    element.height -= deltaY;
-                    element.y += deltaY;
-                  }
+                mutateElement(element, {
+                  x: element.x + deltaX,
+                  y: event.shiftKey
+                    ? element.y + element.height - element.width
+                    : element.y + deltaY,
+                  width: element.width - deltaX,
+                  height: event.shiftKey
+                    ? element.width
+                    : element.height - deltaY,
                 });
               }
               break;
@@ -1522,15 +1522,13 @@ export class App extends React.Component<any, AppState> {
                   event.shiftKey,
                 );
               } else {
-                mutateElement(element, element => {
-                  element.width += deltaX;
-                  if (event.shiftKey) {
-                    element.y += element.height - element.width;
-                    element.height = element.width;
-                  } else {
-                    element.height -= deltaY;
-                    element.y += deltaY;
-                  }
+                const nextWidth = element.width + deltaX;
+                mutateElement(element, {
+                  y: event.shiftKey
+                    ? element.y + element.height - nextWidth
+                    : element.y + deltaY,
+                  width: nextWidth,
+                  height: event.shiftKey ? nextWidth : element.height - deltaY,
                 });
               }
               break;
@@ -1554,14 +1552,12 @@ export class App extends React.Component<any, AppState> {
                   event.shiftKey,
                 );
               } else {
-                mutateElement(element, element => {
-                  element.width -= deltaX;
-                  element.x += deltaX;
-                  if (event.shiftKey) {
-                    element.height = element.width;
-                  } else {
-                    element.height += deltaY;
-                  }
+                mutateElement(element, {
+                  x: element.x + deltaX,
+                  width: element.width - deltaX,
+                  height: event.shiftKey
+                    ? element.width
+                    : element.height + deltaY,
                 });
               }
               break;
@@ -1585,26 +1581,17 @@ export class App extends React.Component<any, AppState> {
                   event.shiftKey,
                 );
               } else {
-                mutateElement(element, element => {
-                  if (event.shiftKey) {
-                    element.width += deltaX;
-                    element.height = element.width;
-                  } else {
-                    element.width += deltaX;
-                    element.height += deltaY;
-                  }
+                mutateElement(element, {
+                  width: element.width + deltaX,
+                  height: event.shiftKey
+                    ? element.width
+                    : element.height + deltaY,
                 });
               }
               break;
             case "n": {
-              mutateElement(element, element => {
-                element.height -= deltaY;
-                element.y += deltaY;
-              });
-
               if (element.points.length > 0) {
                 const len = element.points.length;
-
                 const points = [...element.points].sort((a, b) => a[1] - b[1]);
 
                 for (let i = 1; i < points.length; ++i) {
@@ -1612,14 +1599,14 @@ export class App extends React.Component<any, AppState> {
                   pnt[1] -= deltaY / (len - i);
                 }
               }
+
+              mutateElement(element, {
+                height: element.height - deltaY,
+                y: element.y + deltaY,
+              });
               break;
             }
             case "w": {
-              mutateElement(element, element => {
-                element.width -= deltaX;
-                element.x += deltaX;
-              });
-
               if (element.points.length > 0) {
                 const len = element.points.length;
                 const points = [...element.points].sort((a, b) => a[0] - b[0]);
@@ -1629,39 +1616,44 @@ export class App extends React.Component<any, AppState> {
                   pnt[0] -= deltaX / (len - i);
                 }
               }
+
+              mutateElement(element, {
+                width: element.width - deltaX,
+                x: element.x + deltaX,
+              });
               break;
             }
             case "s": {
-              mutateElement(element, element => {
-                element.height += deltaY;
-                if (element.points.length > 0) {
-                  const len = element.points.length;
-                  const points = [...element.points].sort(
-                    (a, b) => a[1] - b[1],
-                  );
-
-                  for (let i = 1; i < points.length; ++i) {
-                    const pnt = points[i];
-                    pnt[1] += deltaY / (len - i);
-                  }
+              if (element.points.length > 0) {
+                const len = element.points.length;
+                const points = [...element.points].sort((a, b) => a[1] - b[1]);
+
+                for (let i = 1; i < points.length; ++i) {
+                  const pnt = points[i];
+                  pnt[1] += deltaY / (len - i);
                 }
+              }
+
+              mutateElement(element, {
+                height: element.height + deltaY,
+                points: element.points, // no-op, but signifies that we mutated points in-place above
               });
               break;
             }
             case "e": {
-              mutateElement(element, element => {
-                element.width += deltaX;
-                if (element.points.length > 0) {
-                  const len = element.points.length;
-                  const points = [...element.points].sort(
-                    (a, b) => a[0] - b[0],
-                  );
-
-                  for (let i = 1; i < points.length; ++i) {
-                    const pnt = points[i];
-                    pnt[0] += deltaX / (len - i);
-                  }
+              if (element.points.length > 0) {
+                const len = element.points.length;
+                const points = [...element.points].sort((a, b) => a[0] - b[0]);
+
+                for (let i = 1; i < points.length; ++i) {
+                  const pnt = points[i];
+                  pnt[0] += deltaX / (len - i);
                 }
+              }
+
+              mutateElement(element, {
+                width: element.width + deltaX,
+                points: element.points, // no-op, but signifies that we mutated points in-place above
               });
               break;
             }
@@ -1676,9 +1668,9 @@ export class App extends React.Component<any, AppState> {
             element,
             resizeHandle,
           });
-          mutateElement(el, el => {
-            el.x = element.x;
-            el.y = element.y;
+          mutateElement(el, {
+            x: element.x,
+            y: element.y,
           });
           invalidateShapeForElement(el);
 
@@ -1702,9 +1694,9 @@ export class App extends React.Component<any, AppState> {
           );
 
           selectedElements.forEach(element => {
-            mutateElement(element, element => {
-              element.x += x - lastX;
-              element.y += y - lastY;
+            mutateElement(element, {
+              x: element.x + x - lastX,
+              y: element.y + y - lastY,
             });
           });
           lastX = x;
@@ -1767,12 +1759,11 @@ export class App extends React.Component<any, AppState> {
           }
         }
 
-        mutateElement(draggingElement, draggingElement => {
-          draggingElement.x = x < originX ? originX - width : originX;
-          draggingElement.y = y < originY ? originY - height : originY;
-
-          draggingElement.width = width;
-          draggingElement.height = height;
+        mutateElement(draggingElement, {
+          x: x < originX ? originX - width : originX,
+          y: y < originY ? originY - height : originY,
+          width: width,
+          height: height,
         });
       }
 

src/data/restore.ts

@@ -52,6 +52,7 @@ export function restore(
 
       return {
         ...element,
+        version: element.id ? element.version + 1 : element.version || 0,
         id: element.id || nanoid(),
         fillStyle: element.fillStyle || "hachure",
         strokeWidth: element.strokeWidth || 1,

src/element/mutateElement.ts

@@ -1,26 +1,42 @@
-import {
-  MutableExcalidrawElement,
-  MutableExcalidrawTextElement,
-} from "./types";
+import { ExcalidrawElement, ExcalidrawTextElement } from "./types";
+
+type ElementUpdate<TElement extends ExcalidrawElement> = Omit<
+  Partial<TElement>,
+  "id" | "seed"
+>;
 
 // This function tracks updates of text elements for the purposes for collaboration.
 // The version is used to compare updates when more than one user is working in
 // the same drawing.
 export function mutateElement(
-  element: MutableExcalidrawElement,
-  callback: (mutatableElement: MutableExcalidrawElement) => void,
-): void {
-  element.version++;
-  callback(element);
+  element: ExcalidrawElement,
+  updates: ElementUpdate<ExcalidrawElement>,
+) {
+  Object.assign(element, updates);
+  (element as any).version++;
+}
+
+export function newElementWith(
+  element: ExcalidrawElement,
+  updates: ElementUpdate<ExcalidrawElement>,
+): ExcalidrawElement {
+  return { ...element, ...updates, version: element.version + 1 };
 }
 
 // This function tracks updates of text elements for the purposes for collaboration.
 // The version is used to compare updates when more than one user is working in
 // the same document.
 export function mutateTextElement(
-  element: MutableExcalidrawTextElement,
-  callback: (mutatableElement: MutableExcalidrawTextElement) => void,
+  element: ExcalidrawTextElement,
+  updates: ElementUpdate<ExcalidrawTextElement>,
 ): void {
-  element.version++;
-  callback(element);
+  Object.assign(element, updates);
+  (element as any).version++;
+}
+
+export function newTextElementWith(
+  element: ExcalidrawTextElement,
+  updates: ElementUpdate<ExcalidrawTextElement>,
+): ExcalidrawTextElement {
+  return { ...element, ...updates, version: element.version + 1 };
 }

src/element/sizeHelpers.ts

@@ -1,4 +1,4 @@
-import { ExcalidrawElement, MutableExcalidrawElement } from "./types";
+import { ExcalidrawElement } from "./types";
 import { invalidateShapeForElement } from "../renderer/renderElement";
 import { mutateElement } from "./mutateElement";
 
@@ -36,7 +36,7 @@ export function getPerfectElementSize(
 }
 
 export function resizePerfectLineForNWHandler(
-  element: MutableExcalidrawElement,
+  element: ExcalidrawElement,
   x: number,
   y: number,
 ) {
@@ -45,21 +45,28 @@ export function resizePerfectLineForNWHandler(
   const distanceToAnchorX = x - anchorX;
   const distanceToAnchorY = y - anchorY;
   if (Math.abs(distanceToAnchorX) < Math.abs(distanceToAnchorY) / 2) {
-    element.x = anchorX;
-    element.width = 0;
-    element.y = y;
-    element.height = -distanceToAnchorY;
+    mutateElement(element, {
+      x: anchorX,
+      width: 0,
+      y,
+      height: -distanceToAnchorY,
+    });
   } else if (Math.abs(distanceToAnchorY) < Math.abs(element.width) / 2) {
-    element.y = anchorY;
-    element.height = 0;
+    mutateElement(element, {
+      y: anchorY,
+      height: 0,
+    });
   } else {
-    element.x = x;
-    element.width = -distanceToAnchorX;
-    element.height =
+    const nextHeight =
       Math.sign(distanceToAnchorY) *
       Math.sign(distanceToAnchorX) *
       element.width;
-    element.y = anchorY - element.height;
+    mutateElement(element, {
+      x,
+      y: anchorY - nextHeight,
+      width: -distanceToAnchorX,
+      height: nextHeight,
+    });
   }
 }
 
@@ -79,16 +86,18 @@ export function normalizeDimensions(
   }
 
   if (element.width < 0) {
-    mutateElement(element, element => {
-      element.width = Math.abs(element.width);
-      element.x -= element.width;
+    const nextWidth = Math.abs(element.width);
+    mutateElement(element, {
+      width: nextWidth,
+      x: element.x - nextWidth,
     });
   }
 
   if (element.height < 0) {
-    mutateElement(element, element => {
-      element.height = Math.abs(element.height);
-      element.y -= element.height;
+    const nextHeight = Math.abs(element.height);
+    mutateElement(element, {
+      height: nextHeight,
+      y: element.y - nextHeight,
     });
   }
 

src/element/textElement.ts

@@ -1,11 +1,12 @@
 import { measureText } from "../utils";
-import { MutableExcalidrawTextElement } from "./types";
+import { ExcalidrawTextElement } from "./types";
+import { mutateTextElement } from "./mutateElement";
 
-export const redrawTextBoundingBox = (
-  element: MutableExcalidrawTextElement,
-) => {
+export const redrawTextBoundingBox = (element: ExcalidrawTextElement) => {
   const metrics = measureText(element.text, element.font);
-  element.width = metrics.width;
-  element.height = metrics.height;
-  element.baseline = metrics.baseline;
+  mutateTextElement(element, {
+    width: metrics.width,
+    height: metrics.height,
+    baseline: metrics.baseline,
+  });
 };

src/element/types.ts

@@ -6,17 +6,15 @@ import { newElement } from "./newElement";
  * between peers and contain no state local to the peer.
  */
 export type ExcalidrawElement = Readonly<ReturnType<typeof newElement>>;
-export type MutableExcalidrawElement = ReturnType<typeof newElement>;
 
-export type MutableExcalidrawTextElement = MutableExcalidrawElement & {
-  type: "text";
-  font: string;
-  text: string;
-  // for backward compatibility
-  actualBoundingBoxAscent?: number;
-  baseline: number;
-};
-
-export type ExcalidrawTextElement = Readonly<MutableExcalidrawTextElement>;
+export type ExcalidrawTextElement = ExcalidrawElement &
+  Readonly<{
+    type: "text";
+    font: string;
+    text: string;
+    // for backward compatibility
+    actualBoundingBoxAscent?: number;
+    baseline: number;
+  }>;
 
 export type PointerType = "mouse" | "pen" | "touch";

src/history.ts

@@ -1,6 +1,7 @@
 import { AppState } from "./types";
 import { ExcalidrawElement } from "./element/types";
 import { clearAppStatePropertiesForHistory } from "./appState";
+import { newElementWith } from "./element/mutateElement";
 
 type Result = {
   appState: AppState;
@@ -18,13 +19,14 @@ export class SceneHistory {
   ) {
     return JSON.stringify({
       appState: clearAppStatePropertiesForHistory(appState),
-      elements: elements.map(element => ({
-        ...element,
-        points:
-          appState.multiElement && appState.multiElement.id === element.id
-            ? element.points.slice(0, -1)
-            : element.points,
-      })),
+      elements: elements.map(element =>
+        newElementWith(element, {
+          points:
+            appState.multiElement && appState.multiElement.id === element.id
+              ? element.points.slice(0, -1)
+              : element.points,
+        }),
+      ),
     });
   }