diff --git a/src/actions/actionExport.tsx b/src/actions/actionExport.tsx index 0c46736a1b..05d9bc7d4c 100644 --- a/src/actions/actionExport.tsx +++ b/src/actions/actionExport.tsx @@ -201,7 +201,7 @@ export const actionLoadScene = register({ const { elements: loadedElements, appState: loadedAppState, - } = await loadFromJSON(appState); + } = await loadFromJSON(appState, elements); return { elements: loadedElements, appState: loadedAppState, diff --git a/src/components/App.tsx b/src/components/App.tsx index 87a245cafd..4c053d1d41 100644 --- a/src/components/App.tsx +++ b/src/components/App.tsx @@ -654,7 +654,7 @@ class App extends React.Component { const fileHandle = launchParams.files[0]; const blob: Blob = await fileHandle.getFile(); blob.handle = fileHandle; - loadFromBlob(blob, this.state) + loadFromBlob(blob, this.state, this.scene.getElements()) .then(({ elements, appState }) => this.syncActionResult({ elements, @@ -692,7 +692,7 @@ class App extends React.Component { }; } - const scene = restore(initialData, null); + const scene = restore(initialData, null, null); scene.appState = { ...scene.appState, isLoading: false, @@ -1201,7 +1201,7 @@ class App extends React.Component { }); } else if (data.elements) { this.addElementsFromPasteOrLibrary({ - elements: restoreElements(data.elements), + elements: data.elements, position: "cursor", }); } else if (data.text) { @@ -1216,7 +1216,7 @@ class App extends React.Component { elements: readonly ExcalidrawElement[]; position: { clientX: number; clientY: number } | "cursor" | "center"; }) => { - const elements = restoreElements(opts.elements); + const elements = restoreElements(opts.elements, null); const [minX, minY, maxX, maxY] = getCommonBounds(elements); const elementsCenterX = distance(minX, maxX) / 2; @@ -3805,7 +3805,11 @@ class App extends React.Component { try { const file = event.dataTransfer.files[0]; if (file?.type === "image/png" || file?.type === "image/svg+xml") { - const { elements, appState } = await loadFromBlob(file, this.state); + const { elements, appState } = await loadFromBlob( + file, + this.state, + this.scene.getElements(), + ); this.syncActionResult({ elements, appState: { @@ -3865,7 +3869,7 @@ class App extends React.Component { }; loadFileToCanvas = (file: Blob) => { - loadFromBlob(file, this.state) + loadFromBlob(file, this.state, this.scene.getElements()) .then(({ elements, appState }) => this.syncActionResult({ elements, diff --git a/src/data/blob.ts b/src/data/blob.ts index a9a6cefa96..88bae34221 100644 --- a/src/data/blob.ts +++ b/src/data/blob.ts @@ -1,6 +1,7 @@ import { cleanAppStateForExport } from "../appState"; import { EXPORT_DATA_TYPES } from "../constants"; import { clearElementsForExport } from "../element"; +import { ExcalidrawElement } from "../element/types"; import { CanvasError } from "../errors"; import { t } from "../i18n"; import { calculateScrollCenter } from "../scene"; @@ -83,6 +84,7 @@ export const loadFromBlob = async ( blob: Blob, /** @see restore.localAppState */ localAppState: AppState | null, + localElements: readonly ExcalidrawElement[] | null, ) => { const contents = await parseFileContents(blob); try { @@ -103,6 +105,7 @@ export const loadFromBlob = async ( }, }, localAppState, + localElements, ); return result; diff --git a/src/data/json.ts b/src/data/json.ts index e1ed15de60..2b9e91c00a 100644 --- a/src/data/json.ts +++ b/src/data/json.ts @@ -49,7 +49,10 @@ export const saveAsJSON = async ( return { fileHandle }; }; -export const loadFromJSON = async (localAppState: AppState) => { +export const loadFromJSON = async ( + localAppState: AppState, + localElements: readonly ExcalidrawElement[] | null, +) => { const blob = await fileOpen({ description: "Excalidraw files", // ToDo: Be over-permissive until https://bugs.webkit.org/show_bug.cgi?id=34442 @@ -64,7 +67,7 @@ export const loadFromJSON = async (localAppState: AppState) => { ], */ }); - return loadFromBlob(blob, localAppState); + return loadFromBlob(blob, localAppState, localElements); }; export const isValidExcalidrawData = (data?: { diff --git a/src/data/library.ts b/src/data/library.ts index 64ee2c982f..507c349f49 100644 --- a/src/data/library.ts +++ b/src/data/library.ts @@ -18,7 +18,7 @@ class Library { }; restoreLibraryItem = (libraryItem: LibraryItem): LibraryItem | null => { - const elements = getNonDeletedElements(restoreElements(libraryItem)); + const elements = getNonDeletedElements(restoreElements(libraryItem, null)); return elements.length ? elements : null; }; diff --git a/src/data/restore.ts b/src/data/restore.ts index 442f052e82..108be1fdca 100644 --- a/src/data/restore.ts +++ b/src/data/restore.ts @@ -5,7 +5,11 @@ import { } from "../element/types"; import { AppState, NormalizedZoomValue } from "../types"; import { ImportedDataState } from "./types"; -import { getNormalizedDimensions, isInvisiblySmallElement } from "../element"; +import { + getElementMap, + getNormalizedDimensions, + isInvisiblySmallElement, +} from "../element"; import { isLinearElementType } from "../element/typeChecks"; import { randomId } from "../random"; import { @@ -16,6 +20,7 @@ import { } from "../constants"; import { getDefaultAppState } from "../appState"; import { LinearElementEditor } from "../element/linearElementEditor"; +import { bumpVersion } from "../element/mutateElement"; type RestoredAppState = Omit< AppState, @@ -181,13 +186,20 @@ const restoreElement = ( export const restoreElements = ( elements: ImportedDataState["elements"], + /** NOTE doesn't serve for reconciliation */ + localElements: readonly ExcalidrawElement[] | null | undefined, ): ExcalidrawElement[] => { + const localElementsMap = localElements ? getElementMap(localElements) : null; return (elements || []).reduce((elements, element) => { // filtering out selection, which is legacy, no longer kept in elements, // and causing issues if retained if (element.type !== "selection" && !isInvisiblySmallElement(element)) { - const migratedElement = restoreElement(element); + let migratedElement: ExcalidrawElement = restoreElement(element); if (migratedElement) { + const localElement = localElementsMap?.[element.id]; + if (localElement && localElement.version > migratedElement.version) { + migratedElement = bumpVersion(migratedElement, localElement.version); + } elements.push(migratedElement); } } @@ -197,25 +209,25 @@ export const restoreElements = ( export const restoreAppState = ( appState: ImportedDataState["appState"], - localAppState: Partial | null, + localAppState: Partial | null | undefined, ): RestoredAppState => { appState = appState || {}; const defaultAppState = getDefaultAppState(); const nextAppState = {} as typeof defaultAppState; - for (const [key, val] of Object.entries(defaultAppState) as [ + for (const [key, defaultValue] of Object.entries(defaultAppState) as [ keyof typeof defaultAppState, any, ][]) { - const restoredValue = appState[key]; + const suppliedValue = appState[key]; const localValue = localAppState ? localAppState[key] : undefined; (nextAppState as any)[key] = - restoredValue !== undefined - ? restoredValue + suppliedValue !== undefined + ? suppliedValue : localValue !== undefined ? localValue - : val; + : defaultValue; } return { @@ -243,9 +255,10 @@ export const restore = ( * Supply `null` if you can't get access to it. */ localAppState: Partial | null | undefined, + localElements: readonly ExcalidrawElement[] | null | undefined, ): RestoredDataState => { return { - elements: restoreElements(data?.elements), + elements: restoreElements(data?.elements, localElements), appState: restoreAppState(data?.appState, localAppState || null), }; }; diff --git a/src/element/mutateElement.ts b/src/element/mutateElement.ts index 172f12951b..864267d904 100644 --- a/src/element/mutateElement.ts +++ b/src/element/mutateElement.ts @@ -120,8 +120,11 @@ export const newElementWith = ( * * NOTE: does not trigger re-render. */ -export const bumpVersion = (element: Mutable) => { - element.version = element.version + 1; +export const bumpVersion = ( + element: Mutable, + version?: ExcalidrawElement["version"], +) => { + element.version = (version ?? element.version) + 1; element.versionNonce = randomInteger(); return element; }; diff --git a/src/excalidraw-app/data/firebase.ts b/src/excalidraw-app/data/firebase.ts index 66fdd87166..8e92696a08 100644 --- a/src/excalidraw-app/data/firebase.ts +++ b/src/excalidraw-app/data/firebase.ts @@ -196,5 +196,5 @@ export const loadFromFirebase = async ( firebaseSceneVersionCache.set(socket, getSceneVersion(elements)); } - return restoreElements(elements); + return restoreElements(elements, null); }; diff --git a/src/excalidraw-app/data/index.ts b/src/excalidraw-app/data/index.ts index e50dc4cffe..14ea6ba053 100644 --- a/src/excalidraw-app/data/index.ts +++ b/src/excalidraw-app/data/index.ts @@ -257,9 +257,10 @@ export const loadScene = async ( data = restore( await importFromBackend(id, privateKey), localDataState?.appState, + localDataState?.elements, ); } else { - data = restore(localDataState || null, null); + data = restore(localDataState || null, null, null); } return { diff --git a/src/excalidraw-app/index.tsx b/src/excalidraw-app/index.tsx index 91ef63f71e..2f3c241462 100644 --- a/src/excalidraw-app/index.tsx +++ b/src/excalidraw-app/index.tsx @@ -141,7 +141,7 @@ const initializeScene = async (opts: { const url = externalUrlMatch[1]; try { const request = await fetch(window.decodeURIComponent(url)); - const data = await loadFromBlob(await request.blob(), null); + const data = await loadFromBlob(await request.blob(), null, null); if ( !scene.elements.length || window.confirm(t("alerts.loadSceneOverridePrompt")) diff --git a/src/packages/excalidraw/CHANGELOG.md b/src/packages/excalidraw/CHANGELOG.md index dd3a5e2b3f..6a5f7dbc1a 100644 --- a/src/packages/excalidraw/CHANGELOG.md +++ b/src/packages/excalidraw/CHANGELOG.md @@ -19,6 +19,12 @@ Please add the latest change on the top under the correct section. ### Features +- [`restore(data, localAppState, localElements)`](https://github.com/excalidraw/excalidraw/blob/master/src/packages/excalidraw/README.md#restore) and [`restoreElements(elements, localElements)`](https://github.com/excalidraw/excalidraw/blob/master/src/packages/excalidraw/README.md#restoreElements) now take `localElements` argument which will be used to ensure existing elements' versions are used and incremented. This fixes an issue where importing the same file would resolve to elements with older versions, potentially causing issues when reconciling [#3797](https://github.com/excalidraw/excalidraw/pull/3797). + + #### BREAKING CHANGE + + - `localElements` argument is mandatory (can be `null`/`undefined`) if using TypeScript. + - Support `appState.exportEmbedScene` attribute in [`exportToSvg`](https://github.com/excalidraw/excalidraw/blob/master/src/packages/excalidraw/README.md#exportToSvg) which allows to embed the scene data. #### BREAKING CHANGE diff --git a/src/packages/excalidraw/README_NEXT.md b/src/packages/excalidraw/README_NEXT.md index 047eb5e2d7..0f6dbf5d58 100644 --- a/src/packages/excalidraw/README_NEXT.md +++ b/src/packages/excalidraw/README_NEXT.md @@ -701,7 +701,7 @@ This function returns an object where each element is mapped to its id. **_Signature_**
-restoreAppState(appState:  ImportedDataState["appState"], localAppState: Partial<AppState> | null): AppState
+restoreAppState(appState: ImportedDataState["appState"], localAppState: Partial<AppState> | null): AppState
 
**_How to use_** @@ -710,14 +710,16 @@ restoreAppState(appState: -restoreElements(elements: ImportedDataState["elements"]): ExcalidrawElement[] +restoreElements(elements: ImportedDataState["elements"], localElements: ExcalidrawElement[] | null | undefined): ExcalidrawElement[] **_How to use_** @@ -728,21 +730,25 @@ import { restoreElements } from "@excalidraw/excalidraw-next"; This function will make sure all properties of element is correctly set and if any attribute is missing, it will be set to default value. +When `localElements` are supplied, they are used to ensure that existing restored elements reuse `version` (and increment it), and regenerate `versionNonce`. Use this when you import elements which may already be present in the scene to ensure that you do not disregard the newly imported elements if you're using element version to detect the updates. + #### `restore` **_Signature_**
-restoreElements(data:  ImportedDataState): DataState
+restoreElements(data: ImportedDataState, localAppState: Partial<AppState> | null | undefined, localElements: ExcalidrawElement[] | null | undefined): DataState
 
+See [`restoreAppState()`](https://github.com/excalidraw/excalidraw/blob/master/src/packages/excalidraw/README.md#restoreAppState) about `localAppState`, and [`restoreElements()`](https://github.com/excalidraw/excalidraw/blob/master/src/packages/excalidraw/README.md#restoreElements) about `localElements`. + **_How to use_** ```js import { restore } from "@excalidraw/excalidraw-next"; ``` -This function makes sure elements and state is set to appropriate values and set to default value if not present. It is combination of [restoreElements](#restoreElements) and [restoreAppState](#restoreAppState) +This function makes sure elements and state is set to appropriate values and set to default value if not present. It is a combination of [restoreElements](#restoreElements) and [restoreAppState](#restoreAppState). #### `serializeAsJSON` diff --git a/src/packages/utils.ts b/src/packages/utils.ts index 3a21e190f1..28181be3ef 100644 --- a/src/packages/utils.ts +++ b/src/packages/utils.ts @@ -25,6 +25,7 @@ export const exportToCanvas = ({ const { elements: restoredElements, appState: restoredAppState } = restore( { elements, appState }, null, + null, ); const { exportBackground, viewBackgroundColor } = restoredAppState; return _exportToCanvas( @@ -84,6 +85,7 @@ export const exportToSvg = async ({ const { elements: restoredElements, appState: restoredAppState } = restore( { elements, appState }, null, + null, ); return _exportToSvg(getNonDeletedElements(restoredElements), { ...restoredAppState, diff --git a/src/tests/data/restore.test.ts b/src/tests/data/restore.test.ts index 4c0e8d659a..5e88ee9834 100644 --- a/src/tests/data/restore.test.ts +++ b/src/tests/data/restore.test.ts @@ -11,6 +11,7 @@ import { getDefaultAppState } from "../../appState"; import { ImportedDataState } from "../../data/types"; import { NormalizedZoomValue } from "../../types"; import { FONT_FAMILY } from "../../constants"; +import { newElementWith } from "../../element/mutateElement"; const mockSizeHelper = jest.spyOn(sizeHelpers, "isInvisiblySmallElement"); @@ -20,12 +21,12 @@ beforeEach(() => { describe("restoreElements", () => { it("should return empty array when element is null", () => { - expect(restore.restoreElements(null)).toStrictEqual([]); + expect(restore.restoreElements(null, null)).toStrictEqual([]); }); it("should not call isInvisiblySmallElement when element is a selection element", () => { const selectionEl = { type: "selection" } as ExcalidrawElement; - const restoreElements = restore.restoreElements([selectionEl]); + const restoreElements = restore.restoreElements([selectionEl], null); expect(restoreElements.length).toBe(0); expect(sizeHelpers.isInvisiblySmallElement).toBeCalledTimes(0); }); @@ -36,14 +37,16 @@ describe("restoreElements", () => { }); dummyNotSupportedElement.type = "not supported"; - expect(restore.restoreElements([dummyNotSupportedElement]).length).toBe(0); + expect( + restore.restoreElements([dummyNotSupportedElement], null).length, + ).toBe(0); }); it("should return empty array when isInvisiblySmallElement is true", () => { const rectElement = API.createElement({ type: "rectangle" }); mockSizeHelper.mockImplementation(() => true); - expect(restore.restoreElements([rectElement]).length).toBe(0); + expect(restore.restoreElements([rectElement], null).length).toBe(0); }); it("should restore text element correctly passing value for each attribute", () => { @@ -57,9 +60,10 @@ describe("restoreElements", () => { id: "id-text01", }); - const restoredText = restore.restoreElements([ - textElement, - ])[0] as ExcalidrawTextElement; + const restoredText = restore.restoreElements( + [textElement], + null, + )[0] as ExcalidrawTextElement; expect(restoredText).toMatchSnapshot({ seed: expect.any(Number), @@ -77,9 +81,10 @@ describe("restoreElements", () => { textElement.text = null; textElement.font = "10 unknown"; - const restoredText = restore.restoreElements([ - textElement, - ])[0] as ExcalidrawTextElement; + const restoredText = restore.restoreElements( + [textElement], + null, + )[0] as ExcalidrawTextElement; expect(restoredText).toMatchSnapshot({ seed: expect.any(Number), }); @@ -91,9 +96,10 @@ describe("restoreElements", () => { id: "id-freedraw01", }); - const restoredFreedraw = restore.restoreElements([ - freedrawElement, - ])[0] as ExcalidrawFreeDrawElement; + const restoredFreedraw = restore.restoreElements( + [freedrawElement], + null, + )[0] as ExcalidrawFreeDrawElement; expect(restoredFreedraw).toMatchSnapshot({ seed: expect.any(Number) }); }); @@ -107,10 +113,10 @@ describe("restoreElements", () => { }); drawElement.type = "draw"; - const restoredElements = restore.restoreElements([ - lineElement, - drawElement, - ]); + const restoredElements = restore.restoreElements( + [lineElement, drawElement], + null, + ); const restoredLine = restoredElements[0] as ExcalidrawLinearElement; const restoredDraw = restoredElements[1] as ExcalidrawLinearElement; @@ -122,7 +128,7 @@ describe("restoreElements", () => { it("should restore arrow element correctly", () => { const arrowElement = API.createElement({ type: "arrow", id: "id-arrow01" }); - const restoredElements = restore.restoreElements([arrowElement]); + const restoredElements = restore.restoreElements([arrowElement], null); const restoredArrow = restoredElements[0] as ExcalidrawLinearElement; @@ -132,7 +138,7 @@ describe("restoreElements", () => { it("when arrow element has defined endArrowHead", () => { const arrowElement = API.createElement({ type: "arrow" }); - const restoredElements = restore.restoreElements([arrowElement]); + const restoredElements = restore.restoreElements([arrowElement], null); const restoredArrow = restoredElements[0] as ExcalidrawLinearElement; @@ -145,7 +151,7 @@ describe("restoreElements", () => { get: jest.fn(() => undefined), }); - const restoredElements = restore.restoreElements([arrowElement]); + const restoredElements = restore.restoreElements([arrowElement], null); const restoredArrow = restoredElements[0] as ExcalidrawLinearElement; @@ -166,9 +172,10 @@ describe("restoreElements", () => { [lineElement.width, lineElement.height], ]; - const restoredLine = restore.restoreElements([ - lineElement, - ])[0] as ExcalidrawLinearElement; + const restoredLine = restore.restoreElements( + [lineElement], + null, + )[0] as ExcalidrawLinearElement; expect(restoredLine.points).toMatchObject(expectedLinePoints); }); @@ -205,10 +212,10 @@ describe("restoreElements", () => { get: jest.fn(() => pointsEl_1), }); - const restoredElements = restore.restoreElements([ - lineElement_0, - lineElement_1, - ]); + const restoredElements = restore.restoreElements( + [lineElement_0, lineElement_1], + null, + ); const restoredLine_0 = restoredElements[0] as ExcalidrawLinearElement; const restoredLine_1 = restoredElements[1] as ExcalidrawLinearElement; @@ -254,12 +261,37 @@ describe("restoreElements", () => { elements.push(element); }); - const restoredElements = restore.restoreElements(elements); + const restoredElements = restore.restoreElements(elements, null); expect(restoredElements[0]).toMatchSnapshot({ seed: expect.any(Number) }); expect(restoredElements[1]).toMatchSnapshot({ seed: expect.any(Number) }); expect(restoredElements[2]).toMatchSnapshot({ seed: expect.any(Number) }); }); + + it("bump versions of local duplicate elements when supplied", () => { + const rectangle = API.createElement({ type: "rectangle" }); + const ellipse = API.createElement({ type: "ellipse" }); + const rectangle_modified = newElementWith(rectangle, { isDeleted: true }); + + const restoredElements = restore.restoreElements( + [rectangle, ellipse], + [rectangle_modified], + ); + + expect(restoredElements[0].id).toBe(rectangle.id); + expect(restoredElements[0].versionNonce).not.toBe(rectangle.versionNonce); + expect(restoredElements).toEqual([ + expect.objectContaining({ + id: rectangle.id, + version: rectangle_modified.version + 1, + }), + expect.objectContaining({ + id: ellipse.id, + version: ellipse.version, + versionNonce: ellipse.versionNonce, + }), + ]); + }); }); describe("restoreAppState", () => { @@ -429,7 +461,7 @@ describe("restore", () => { it("when imported data state is null it should return an empty array of elements", () => { const stubLocalAppState = getDefaultAppState(); - const restoredData = restore.restore(null, stubLocalAppState); + const restoredData = restore.restore(null, stubLocalAppState, null); expect(restoredData.elements.length).toBe(0); }); @@ -438,7 +470,7 @@ describe("restore", () => { stubLocalAppState.cursorButton = "down"; stubLocalAppState.name = "local app state"; - const restoredData = restore.restore(null, stubLocalAppState); + const restoredData = restore.restore(null, stubLocalAppState, null); expect(restoredData.appState.cursorButton).toBe( stubLocalAppState.cursorButton, ); @@ -455,7 +487,11 @@ describe("restore", () => { const importedDataState = {} as ImportedDataState; importedDataState.elements = elements; - const restoredData = restore.restore(importedDataState, stubLocalAppState); + const restoredData = restore.restore( + importedDataState, + stubLocalAppState, + null, + ); expect(restoredData.elements.length).toBe(elements.length); }); @@ -467,10 +503,36 @@ describe("restore", () => { const importedDataState = {} as ImportedDataState; importedDataState.appState = stubImportedAppState; - const restoredData = restore.restore(importedDataState, null); + const restoredData = restore.restore(importedDataState, null, null); expect(restoredData.appState.cursorButton).toBe( stubImportedAppState.cursorButton, ); expect(restoredData.appState.name).toBe(stubImportedAppState.name); }); + + it("bump versions of local duplicate elements when supplied", () => { + const rectangle = API.createElement({ type: "rectangle" }); + const ellipse = API.createElement({ type: "ellipse" }); + + const rectangle_modified = newElementWith(rectangle, { isDeleted: true }); + + const restoredData = restore.restore( + { elements: [rectangle, ellipse] }, + null, + [rectangle_modified], + ); + + expect(restoredData.elements[0].id).toBe(rectangle.id); + expect(restoredData.elements[0].versionNonce).not.toBe( + rectangle.versionNonce, + ); + expect(restoredData.elements).toEqual([ + expect.objectContaining({ version: rectangle_modified.version + 1 }), + expect.objectContaining({ + id: ellipse.id, + version: ellipse.version, + versionNonce: ellipse.versionNonce, + }), + ]); + }); });