From 6c908553a9cbd2e50bfabc49bc6045c5ba97d702 Mon Sep 17 00:00:00 2001 From: David Luzar <5153846+dwelle@users.noreply.github.com> Date: Sun, 4 Jan 2026 15:13:38 +0100 Subject: [PATCH] fix: reconciliation of server updates & refactor restore (#10597) --- excalidraw-app/App.tsx | 48 ++++- excalidraw-app/collab/Collab.tsx | 39 +++-- excalidraw-app/data/firebase.ts | 4 +- excalidraw-app/data/index.ts | 46 +---- packages/common/src/utils.ts | 64 +++++-- packages/excalidraw/components/App.tsx | 35 ++-- packages/excalidraw/data/blob.ts | 31 ++-- packages/excalidraw/data/reconcile.ts | 2 +- packages/excalidraw/data/restore.ts | 140 +++++++++------ packages/excalidraw/index.tsx | 1 - .../excalidraw/tests/data/restore.test.ts | 165 +++++++++--------- packages/utils/src/export.ts | 26 +-- 12 files changed, 342 insertions(+), 259 deletions(-) diff --git a/excalidraw-app/App.tsx b/excalidraw-app/App.tsx index 94ab8e4e3b..2e71cc6788 100644 --- a/excalidraw-app/App.tsx +++ b/excalidraw-app/App.tsx @@ -48,7 +48,11 @@ import { youtubeIcon, } from "@excalidraw/excalidraw/components/icons"; import { isElementLink } from "@excalidraw/element"; -import { restore, restoreAppState } from "@excalidraw/excalidraw/data/restore"; +import { + bumpElementVersions, + restoreAppState, + restoreElements, +} from "@excalidraw/excalidraw/data/restore"; import { newElementWith } from "@excalidraw/element"; import { isInitializedImageElement } from "@excalidraw/element"; import clsx from "clsx"; @@ -105,8 +109,8 @@ import { TopErrorBoundary } from "./components/TopErrorBoundary"; import { exportToBackend, getCollaborationLinkData, + importFromBackend, isCollaborationLink, - loadScene, } from "./data"; import { updateStaleImageStatuses } from "./data/FileManager"; @@ -224,9 +228,20 @@ const initializeScene = async (opts: { const localDataState = importFromLocalStorage(); - let scene: RestoredDataState & { + let scene: Omit< + RestoredDataState, + // we're not storing files in the scene database/localStorage, and instead + // fetch them async from a different store + "files" + > & { scrollToContent?: boolean; - } = await loadScene(null, null, localDataState); + } = { + elements: restoreElements(localDataState?.elements, null, { + repairBindings: true, + deleteInvisibleElements: true, + }), + appState: restoreAppState(localDataState?.appState, null), + }; let roomLinkData = getCollaborationLinkData(window.location.href); const isExternalScene = !!(id || jsonBackendMatch || roomLinkData); @@ -240,11 +255,26 @@ const initializeScene = async (opts: { (await openConfirmModal(shareableLinkConfirmDialog)) ) { if (jsonBackendMatch) { - scene = await loadScene( + const imported = await importFromBackend( jsonBackendMatch[1], jsonBackendMatch[2], - localDataState, ); + + scene = { + elements: bumpElementVersions( + restoreElements(imported.elements, null, { + repairBindings: true, + deleteInvisibleElements: true, + }), + localDataState?.elements, + ), + appState: restoreAppState( + imported.appState, + // local appState when importing from backend to ensure we restore + // localStorage user settings which we do not persist on server. + localDataState?.appState, + ), + }; } scene.scrollToContent = true; if (!roomLinkData) { @@ -496,8 +526,10 @@ const ExcalidrawWrapper = () => { loadImages(data); if (data.scene) { excalidrawAPI.updateScene({ - ...data.scene, - ...restore(data.scene, null, null, { repairBindings: true }), + elements: restoreElements(data.scene.elements, null, { + repairBindings: true, + }), + appState: restoreAppState(data.scene.appState, null), captureUpdate: CaptureUpdateAction.IMMEDIATELY, }); } diff --git a/excalidraw-app/collab/Collab.tsx b/excalidraw-app/collab/Collab.tsx index 995c5b8891..6eef30d7f1 100644 --- a/excalidraw-app/collab/Collab.tsx +++ b/excalidraw-app/collab/Collab.tsx @@ -6,7 +6,7 @@ import { reconcileElements, } from "@excalidraw/excalidraw"; import { ErrorDialog } from "@excalidraw/excalidraw/components/ErrorDialog"; -import { APP_NAME, EVENT } from "@excalidraw/common"; +import { APP_NAME, cloneJSON, EVENT, toBrandedType } from "@excalidraw/common"; import { IDLE_THRESHOLD, ACTIVE_THRESHOLD, @@ -29,6 +29,8 @@ import { withBatchedUpdates } from "@excalidraw/excalidraw/reactUtils"; import throttle from "lodash.throttle"; import { PureComponent } from "react"; +import { bumpElementVersions } from "@excalidraw/excalidraw/data/restore"; + import type { ReconciledExcalidrawElement, RemoteExcalidrawElement, @@ -311,6 +313,7 @@ class Collab extends PureComponent { saveCollabRoomToFirebase = async ( syncableElements: readonly SyncableExcalidrawElement[], ) => { + syncableElements = cloneJSON(syncableElements); try { const storedElements = await saveToFirebase( this.portal, @@ -579,7 +582,9 @@ class Collab extends PureComponent { case WS_SUBTYPES.INIT: { if (!this.portal.socketInitialized) { this.initializeRoom({ fetchScene: false }); - const remoteElements = decryptedData.payload.elements; + const remoteElements = toBrandedType< + readonly RemoteExcalidrawElement[] + >(decryptedData.payload.elements); const reconciledElements = this._reconcileElements(remoteElements); this.handleRemoteSceneUpdate(reconciledElements); @@ -593,7 +598,11 @@ class Collab extends PureComponent { } case WS_SUBTYPES.UPDATE: this.handleRemoteSceneUpdate( - this._reconcileElements(decryptedData.payload.elements), + this._reconcileElements( + toBrandedType( + decryptedData.payload.elements, + ), + ), ); break; case WS_SUBTYPES.MOUSE_LOCATION: { @@ -742,20 +751,28 @@ class Collab extends PureComponent { }; private _reconcileElements = ( - remoteElements: readonly ExcalidrawElement[], + remoteElements: readonly RemoteExcalidrawElement[], ): ReconciledExcalidrawElement[] => { - const localElements = this.getSceneElementsIncludingDeleted(); const appState = this.excalidrawAPI.getAppState(); - const restoredRemoteElements = restoreElements( + + const existingElements = this.getSceneElementsIncludingDeleted(); + + // NOTE ideally we restore _after_ reconciliation but we can't do that + // as we'd regenerate even elements such as appState.newElement which would + // break the state + remoteElements = restoreElements(remoteElements, existingElements); + + let reconciledElements = reconcileElements( + existingElements, remoteElements, - this.excalidrawAPI.getSceneElementsMapIncludingDeleted(), - ); - const reconciledElements = reconcileElements( - localElements, - restoredRemoteElements as RemoteExcalidrawElement[], appState, ); + reconciledElements = bumpElementVersions( + reconciledElements, + existingElements, + ); + // Avoid broadcasting to the rest of the collaborators the scene // we just received! // Note: this needs to be set before updating the scene as it diff --git a/excalidraw-app/data/firebase.ts b/excalidraw-app/data/firebase.ts index da25271ca6..11177d90aa 100644 --- a/excalidraw-app/data/firebase.ts +++ b/excalidraw-app/data/firebase.ts @@ -1,5 +1,5 @@ import { reconcileElements } from "@excalidraw/excalidraw"; -import { MIME_TYPES } from "@excalidraw/common"; +import { MIME_TYPES, toBrandedType } from "@excalidraw/common"; import { decompressData } from "@excalidraw/excalidraw/data/encode"; import { encryptData, @@ -243,7 +243,7 @@ export const saveToFirebase = async ( FirebaseSceneVersionCache.set(socket, storedElements); - return storedElements; + return toBrandedType(storedElements); }; export const loadFromFirebase = async ( diff --git a/excalidraw-app/data/index.ts b/excalidraw-app/data/index.ts index cc7d5e8cc9..bf765941a2 100644 --- a/excalidraw-app/data/index.ts +++ b/excalidraw-app/data/index.ts @@ -8,7 +8,6 @@ import { IV_LENGTH_BYTES, } from "@excalidraw/excalidraw/data/encryption"; import { serializeAsJSON } from "@excalidraw/excalidraw/data/json"; -import { restore } from "@excalidraw/excalidraw/data/restore"; import { isInvisiblySmallElement } from "@excalidraw/element"; import { isInitializedImageElement } from "@excalidraw/element"; import { t } from "@excalidraw/excalidraw/i18n"; @@ -84,13 +83,13 @@ export type SocketUpdateDataSource = { SCENE_INIT: { type: WS_SUBTYPES.INIT; payload: { - elements: readonly ExcalidrawElement[]; + elements: readonly OrderedExcalidrawElement[]; }; }; SCENE_UPDATE: { type: WS_SUBTYPES.UPDATE; payload: { - elements: readonly ExcalidrawElement[]; + elements: readonly OrderedExcalidrawElement[]; }; }; MOUSE_LOCATION: { @@ -200,7 +199,7 @@ const legacy_decodeFromBackend = async ({ }; }; -const importFromBackend = async ( +export const importFromBackend = async ( id: string, decryptionKey: string, ): Promise => { @@ -242,45 +241,6 @@ const importFromBackend = async ( } }; -export const loadScene = async ( - id: string | null, - privateKey: string | null, - // Supply local state even if importing from backend to ensure we restore - // localStorage user settings which we do not persist on server. - // Non-optional so we don't forget to pass it even if `undefined`. - localDataState: ImportedDataState | undefined | null, -) => { - let data; - if (id != null && privateKey != null) { - // the private key is used to decrypt the content from the server, take - // extra care not to leak it - data = restore( - await importFromBackend(id, privateKey), - localDataState?.appState, - localDataState?.elements, - { - repairBindings: true, - refreshDimensions: false, - deleteInvisibleElements: true, - }, - ); - } else { - data = restore(localDataState || null, null, null, { - repairBindings: true, - deleteInvisibleElements: true, - }); - } - - return { - elements: data.elements, - appState: data.appState, - // note: this will always be empty because we're not storing files - // in the scene database/localStorage, and instead fetch them async - // from a different database - files: data.files, - }; -}; - type ExportToBackendResult = | { url: null; errorMessage: string } | { url: string; errorMessage: null }; diff --git a/packages/common/src/utils.ts b/packages/common/src/utils.ts index 356f951521..7cb7d99133 100644 --- a/packages/common/src/utils.ts +++ b/packages/common/src/utils.ts @@ -1157,39 +1157,69 @@ export const normalizeEOL = (str: string) => { }; // ----------------------------------------------------------------------------- -type HasBrand = { +export type HasBrand = { // eslint-disable-next-line @typescript-eslint/no-unused-vars - [K in keyof T]: K extends `~brand${infer _}` ? true : never; + [K in keyof T]: K extends `~brand${infer _}` | "_brand" ? true : never; }[keyof T]; type RemoveAllBrands = HasBrand extends true ? { // eslint-disable-next-line @typescript-eslint/no-unused-vars - [K in keyof T as K extends `~brand~${infer _}` ? never : K]: T[K]; + [K in keyof T as K extends `~brand~${infer _}` | "_brand" + ? never + : K]: T[K]; } - : never; + : T; -// adapted from https://github.com/colinhacks/zod/discussions/1994#discussioncomment-6068940 -// currently does not cover all types (e.g. tuples, promises...) -type Unbrand = T extends Map - ? Map +// For accepting values - uses loose matching for branded types +// Preserves readonly modifier: mutable array requires mutable input +type UnbrandForValue = T extends Map + ? Map, UnbrandForValue> : T extends Set - ? Set - : T extends Array - ? Array + ? Set> + : T extends readonly any[] + ? T extends any[] + ? unknown[] // mutable array - require mutable input + : readonly unknown[] // readonly array - accept readonly input : RemoveAllBrands; +// For return types - preserves array element unbranding +export type Unbrand = T extends Map + ? Map, Unbrand> + : T extends Set + ? Set> + : T extends readonly (infer E)[] + ? Array> + : RemoveAllBrands; + +export type CombineBrands = + BrandedType extends readonly (infer BE)[] + ? CurrentType extends readonly (infer CE)[] + ? Array + : CurrentType & BrandedType + : CurrentType & BrandedType; + +export type CombineBrandsIfNeeded = [T] extends [Required] + ? T[] + : HasBrand extends true + ? CombineBrands[] + : Required[]; + /** * Makes type into a branded type, ensuring that value is assignable to - * the base ubranded type. Optionally you can explicitly supply current value + * the base unbranded type. Optionally you can explicitly supply current value * type to combine both (useful for composite branded types. Make sure you * compose branded types which are not composite themselves.) */ -export const toBrandedType = ( - value: Unbrand, -) => { - return value as CurrentType & BrandedType; -}; +export function toBrandedType( + value: UnbrandForValue, +): BrandedType; +export function toBrandedType( + value: CurrentType, +): CombineBrands; +export function toBrandedType(value: unknown) { + return value; +} // ----------------------------------------------------------------------------- diff --git a/packages/excalidraw/components/App.tsx b/packages/excalidraw/components/App.tsx index 4e4f78fbd7..8b9a1a5139 100644 --- a/packages/excalidraw/components/App.tsx +++ b/packages/excalidraw/components/App.tsx @@ -346,7 +346,7 @@ import { import { exportCanvas, loadFromBlob } from "../data"; import Library, { distributeLibraryItemsOnSquareGrid } from "../data/library"; -import { restore, restoreElements } from "../data/restore"; +import { restoreAppState, restoreElements } from "../data/restore"; import { getCenter, getDistance } from "../gesture"; import { History } from "../history"; import { defaultLang, getLanguage, languages, setLanguage, t } from "../i18n"; @@ -2701,46 +2701,47 @@ class App extends React.Component { }, }; } - const scene = restore(initialData, null, null, { + const restoredElements = restoreElements(initialData?.elements, null, { repairBindings: true, deleteInvisibleElements: true, }); - const activeTool = scene.appState.activeTool; + let restoredAppState = restoreAppState(initialData?.appState, null); + const activeTool = restoredAppState.activeTool; - if (!scene.appState.preferredSelectionTool.initialized) { - scene.appState.preferredSelectionTool = { + if (!restoredAppState.preferredSelectionTool.initialized) { + restoredAppState.preferredSelectionTool = { type: this.editorInterface.formFactor === "phone" ? "lasso" : "selection", initialized: true, }; } - scene.appState = { - ...scene.appState, - theme: this.props.theme || scene.appState.theme, + restoredAppState = { + ...restoredAppState, + theme: this.props.theme || restoredAppState.theme, // we're falling back to current (pre-init) state when deciding // whether to open the library, to handle a case where we // update the state outside of initialData (e.g. when loading the app // with a library install link, which should auto-open the library) - openSidebar: scene.appState?.openSidebar || this.state.openSidebar, + openSidebar: restoredAppState?.openSidebar || this.state.openSidebar, activeTool: activeTool.type === "image" || activeTool.type === "lasso" || activeTool.type === "selection" ? { ...activeTool, - type: scene.appState.preferredSelectionTool.type, + type: restoredAppState.preferredSelectionTool.type, } - : scene.appState.activeTool, + : restoredAppState.activeTool, isLoading: false, toast: this.state.toast, }; if (initialData?.scrollToContent) { - scene.appState = { - ...scene.appState, - ...calculateScrollCenter(scene.elements, { - ...scene.appState, + restoredAppState = { + ...restoredAppState, + ...calculateScrollCenter(restoredElements, { + ...restoredAppState, width: this.state.width, height: this.state.height, offsetTop: this.state.offsetTop, @@ -2752,7 +2753,9 @@ class App extends React.Component { this.resetStore(); this.resetHistory(); this.syncActionResult({ - ...scene, + elements: restoredElements, + appState: restoredAppState, + files: initialData?.files, captureUpdate: CaptureUpdateAction.NEVER, }); diff --git a/packages/excalidraw/data/blob.ts b/packages/excalidraw/data/blob.ts index 4de63d645f..c520c33b1c 100644 --- a/packages/excalidraw/data/blob.ts +++ b/packages/excalidraw/data/blob.ts @@ -19,7 +19,11 @@ import { decodeSvgBase64Payload } from "../scene/export"; import { base64ToString, stringToBase64, toByteString } from "./encode"; import { nativeFileSystemSupported } from "./filesystem"; import { isValidExcalidrawData, isValidLibrary } from "./json"; -import { restore, restoreLibraryItems } from "./restore"; +import { + restoreAppState, + restoreElements, + restoreLibraryItems, +} from "./restore"; import type { AppState, DataURL, LibraryItem } from "../types"; @@ -155,10 +159,13 @@ export const loadSceneOrLibraryFromBlob = async ( if (isValidExcalidrawData(data)) { return { type: MIME_TYPES.excalidraw, - data: restore( - { - elements: data.elements || [], - appState: { + data: { + elements: restoreElements(data.elements, localElements, { + repairBindings: true, + deleteInvisibleElements: true, + }), + appState: restoreAppState( + { theme: localAppState?.theme, fileHandle: fileHandle || blob.handle || null, ...cleanAppStateForExport(data.appState || {}), @@ -166,16 +173,10 @@ export const loadSceneOrLibraryFromBlob = async ( ? calculateScrollCenter(data.elements || [], localAppState) : {}), }, - files: data.files, - }, - localAppState, - localElements, - { - repairBindings: true, - refreshDimensions: false, - deleteInvisibleElements: true, - }, - ), + localAppState, + ), + files: data.files || {}, + }, }; } else if (isValidLibrary(data)) { return { diff --git a/packages/excalidraw/data/reconcile.ts b/packages/excalidraw/data/reconcile.ts index a93f951003..7cb702cff9 100644 --- a/packages/excalidraw/data/reconcile.ts +++ b/packages/excalidraw/data/reconcile.ts @@ -36,7 +36,7 @@ export const shouldDiscardRemoteElement = ( // resolve conflicting edits deterministically by taking the one with // the lowest versionNonce (local.version === remote.version && - local.versionNonce < remote.versionNonce)) + local.versionNonce <= remote.versionNonce)) ) { return true; } diff --git a/packages/excalidraw/data/restore.ts b/packages/excalidraw/data/restore.ts index 9011783ca4..31e3563dff 100644 --- a/packages/excalidraw/data/restore.ts +++ b/packages/excalidraw/data/restore.ts @@ -1,6 +1,7 @@ import { isFiniteNumber, pointFrom } from "@excalidraw/math"; import { + type CombineBrandsIfNeeded, DEFAULT_FONT_FAMILY, DEFAULT_TEXT_ALIGN, DEFAULT_VERTICAL_ALIGN, @@ -131,13 +132,18 @@ const repairBinding = ( element: T, binding: FixedPointBinding | null, targetElementsMap: Readonly, - localElementsMap: Readonly | null | undefined, + /** used for context (arrow bindings) */ + existingElementsMap: Readonly | null | undefined, startOrEnd: "start" | "end", ): FixedPointBinding | null => { if (!binding) { return null; } + // --------------------------------------------------------------------------- + // elbow arrows + // --------------------------------------------------------------------------- + if (isElbowArrow(element)) { const fixedPointBinding: | ExcalidrawElbowArrowElement["startBinding"] @@ -150,24 +156,41 @@ const repairBinding = ( return fixedPointBinding; } - // Fallback if the bound element is missing but the binding is at least - // looking like a valid one shape-wise - if (binding.mode && binding.fixedPoint && binding.elementId) { - return { - elementId: binding.elementId, - mode: binding.mode, - fixedPoint: normalizeFixedPoint(binding.fixedPoint || [0.5, 0.5]), - } as FixedPointBinding | null; + // --------------------------------------------------------------------------- + // simple arrows + // --------------------------------------------------------------------------- + + // binding schema v2 + // --------------------------------------------------------------------------- + + if (binding.mode) { + // if latest binding schema, don't check if binding.elementId exists + // (it's done in a separate pass) + if (binding.elementId) { + return { + elementId: binding.elementId, + mode: binding.mode, + fixedPoint: normalizeFixedPoint(binding.fixedPoint || [0.5, 0.5]), + } as FixedPointBinding | null; + } + return null; } + // binding schema v1 (legacy) -> attempt to migrate to v2 + // --------------------------------------------------------------------------- + const targetBoundElement = (targetElementsMap.get(binding.elementId) as ExcalidrawBindableElement) || undefined; const boundElement = targetBoundElement || - (localElementsMap?.get(binding.elementId) as ExcalidrawBindableElement) || + (existingElementsMap?.get( + binding.elementId, + ) as ExcalidrawBindableElement) || undefined; - const elementsMap = targetBoundElement ? targetElementsMap : localElementsMap; + const elementsMap = targetBoundElement + ? targetElementsMap + : existingElementsMap; // migrating legacy focus point bindings if (boundElement && elementsMap) { @@ -296,9 +319,12 @@ const restoreElementWithProperties = < }; export const restoreElement = ( + /** element to be restored */ element: Exclude, + /** all elements to be restored */ targetElementsMap: Readonly, - localElementsMap: Readonly | null | undefined, + /** used for additional context */ + existingElementsMap: Readonly | null | undefined, opts?: { deleteInvisibleElements?: boolean; }, @@ -420,14 +446,14 @@ export const restoreElement = ( element as ExcalidrawArrowElement, element.startBinding, targetElementsMap, - localElementsMap, + existingElementsMap, "start", ), endBinding: repairBinding( element as ExcalidrawArrowElement, element.endBinding, targetElementsMap, - localElementsMap, + existingElementsMap, "end", ), startArrowhead, @@ -588,10 +614,10 @@ const repairFrameMembership = ( } }; -export const restoreElements = ( - targetElements: ImportedDataState["elements"], - /** NOTE doesn't serve for reconciliation */ - localElements: Readonly | null | undefined, +export const restoreElements = ( + targetElements: readonly T[] | undefined | null, + /** used for additional context (e.g. repairing arrow bindings) */ + existingElements: Readonly | null | undefined, opts?: | { refreshDimensions?: boolean; @@ -599,11 +625,13 @@ export const restoreElements = ( deleteInvisibleElements?: boolean; } | undefined, -): OrderedExcalidrawElement[] => { +): CombineBrandsIfNeeded => { // used to detect duplicate top-level element ids const existingIds = new Set(); const targetElementsMap = arrayToMap(targetElements || []); - const localElementsMap = localElements ? arrayToMap(localElements) : null; + const existingElementsMap = existingElements + ? arrayToMap(existingElements) + : null; const restoredElements = syncInvalidIndices( (targetElements || []).reduce((elements, element) => { // filtering out selection, which is legacy, no longer kept in elements, @@ -615,21 +643,18 @@ export const restoreElements = ( let migratedElement: ExcalidrawElement | null = restoreElement( element, targetElementsMap, - localElementsMap, + existingElementsMap, { deleteInvisibleElements: opts?.deleteInvisibleElements, }, ); if (migratedElement) { - const localElement = localElementsMap?.get(element.id); + const localElement = existingElementsMap?.get(element.id); const shouldMarkAsDeleted = opts?.deleteInvisibleElements && isInvisiblySmallElement(element); - if ( - shouldMarkAsDeleted || - (localElement && localElement.version > migratedElement.version) - ) { + if (shouldMarkAsDeleted) { migratedElement = bumpVersion(migratedElement, localElement?.version); } @@ -650,7 +675,10 @@ export const restoreElements = ( ); if (!opts?.repairBindings) { - return restoredElements; + return restoredElements as CombineBrandsIfNeeded< + T, + OrderedExcalidrawElement + >; } // repair binding. Mutates elements. @@ -759,6 +787,41 @@ export const restoreElements = ( }; } + return element; + }) as CombineBrandsIfNeeded; +}; + +/** + * When replacing elements that may exist locally, this bumps their versions + * to the local version + 1. Mainly for later reconciliation to work properly. + * + * See https://github.com/excalidraw/excalidraw/issues/3795 + * + * Generally use this on editor boundaries (importing from file etc.), though + * it does not apply universally (e.g. we don't want to do this for collab + * updates). + */ +export const bumpElementVersions = ( + targetElements: readonly T[], + localElements: Readonly | null | undefined, +) => { + const localElementsMap = localElements ? arrayToMap(localElements) : null; + + return targetElements.map((element) => { + const localElement = localElementsMap?.get(element.id); + + if ( + localElement && + (localElement.version > element.version || + // same versions but different versionNonce means different edits + // (this often means the element was bumped during restore e.g. due + // to re-indexing, and the original element was modified elsewhere + // and supplied as localElements) + (localElement.version === element.version && + localElement.versionNonce !== element.versionNonce)) + ) { + return bumpVersion(element, localElement.version); + } return element; }); }; @@ -875,29 +938,6 @@ export const restoreAppState = ( }; }; -export const restore = ( - data: Pick | null, - /** - * Local AppState (`this.state` or initial state from localStorage) so that we - * don't overwrite local state with default values (when values not - * explicitly specified). - * Supply `null` if you can't get access to it. - */ - localAppState: Partial | null | undefined, - localElements: readonly ExcalidrawElement[] | null | undefined, - elementsConfig?: { - refreshDimensions?: boolean; - repairBindings?: boolean; - deleteInvisibleElements?: boolean; - }, -): RestoredDataState => { - return { - elements: restoreElements(data?.elements, localElements, elementsConfig), - appState: restoreAppState(data?.appState, localAppState || null), - files: data?.files || {}, - }; -}; - const restoreLibraryItem = (libraryItem: LibraryItem) => { const elements = restoreElements( getNonDeletedElements(libraryItem.elements), diff --git a/packages/excalidraw/index.tsx b/packages/excalidraw/index.tsx index d6ce2ecd92..c6fc09846a 100644 --- a/packages/excalidraw/index.tsx +++ b/packages/excalidraw/index.tsx @@ -229,7 +229,6 @@ export { isInvisiblySmallElement } from "@excalidraw/element"; export { defaultLang, useI18n, languages } from "./i18n"; export { - restore, restoreAppState, restoreElement, restoreElements, diff --git a/packages/excalidraw/tests/data/restore.test.ts b/packages/excalidraw/tests/data/restore.test.ts index 3eee14d4a4..d666e48e11 100644 --- a/packages/excalidraw/tests/data/restore.test.ts +++ b/packages/excalidraw/tests/data/restore.test.ts @@ -34,6 +34,20 @@ describe("restoreElements", () => { mockSizeHelper.mockRestore(); }); + it("basic restoreElements", () => { + const textElement = API.createElement({ type: "text" }); + const rectElement = API.createElement({ type: "rectangle" }); + const elements = [textElement, rectElement]; + + const restoredElements = restore.restoreElements(elements, null); + expect(restoredElements.length).toBe(elements.length); + }); + + it("when imported data state is null it should return an empty array of elements", () => { + const restoredElements = restore.restoreElements(null, null); + expect(restoredElements.length).toBe(0); + }); + it("should return empty array when element is null", () => { expect(restore.restoreElements(null, null)).toStrictEqual([]); }); @@ -434,12 +448,12 @@ describe("restoreElements", () => { }); it("bump versions of local duplicate elements when supplied", () => { - const rectangle = API.createElement({ type: "rectangle" }); + const rectangle = API.createElement({ type: "rectangle" }); // version=1 const ellipse = API.createElement({ type: "ellipse" }); - const rectangle_modified = newElementWith(rectangle, { isDeleted: true }); + const rectangle_modified = newElementWith(rectangle, { isDeleted: true }); // version=2 - const restoredElements = restore.restoreElements( - [rectangle, ellipse], + const restoredElements = restore.bumpElementVersions( + restore.restoreElements([rectangle, ellipse], null), [rectangle_modified], ); @@ -448,7 +462,7 @@ describe("restoreElements", () => { expect(restoredElements).toEqual([ expect.objectContaining({ id: rectangle.id, - version: rectangle_modified.version + 2, + version: rectangle_modified.version + 1, }), expect.objectContaining({ id: ellipse.id, @@ -456,9 +470,73 @@ describe("restoreElements", () => { }), ]); }); + + it("bump versions of local duplicate elements when supplied even if both have same version", () => { + const rectangle = API.createElement({ type: "rectangle" }); + + const restored_rectangle_1 = restore.restoreElements([rectangle], null)[0]; + const restored_rectangle_2 = restore.restoreElements( + [restored_rectangle_1], + null, + )[0]; + + // restored rectangle version should be +1 because of re-index + expect(rectangle.version).not.toBe(restored_rectangle_1.version); + + // restoring it again shouldn't re-index again + expect(restored_rectangle_1.version).toBe(restored_rectangle_2.version); + expect(restored_rectangle_1.versionNonce).toBe( + restored_rectangle_2.versionNonce, + ); + + const modified_rectangle_1 = newElementWith(restored_rectangle_1, { + width: 500, + }); + const modified_rectangle_2 = newElementWith(restored_rectangle_2, { + width: 600, + }); + + const restoredElements = restore.bumpElementVersions( + restore.restoreElements([modified_rectangle_1], null), + [modified_rectangle_2], + ); + + expect(restoredElements[0].id).toBe(rectangle.id); + expect(restoredElements[0].id).toBe(modified_rectangle_1.id); + expect(restoredElements[0].versionNonce).not.toBe( + modified_rectangle_1.versionNonce, + ); + expect(restoredElements[0].version).toBe(modified_rectangle_2.version + 1); + }); }); describe("restoreAppState", () => { + it("when appState is null it should return the local app state property", () => { + const stubLocalAppState = getDefaultAppState(); + stubLocalAppState.cursorButton = "down"; + stubLocalAppState.name = "local app state"; + + const restoredAppState = restore.restoreAppState(null, stubLocalAppState); + expect(restoredAppState.cursorButton).toBe(stubLocalAppState.cursorButton); + expect(restoredAppState.name).toBe(stubLocalAppState.name); + }); + + it("when local appState is null but imported app state is supplied", () => { + const stubImportedAppState = getDefaultAppState(); + stubImportedAppState.cursorButton = "down"; + stubImportedAppState.name = "imported app state"; + + const importedDataState = {} as ImportedDataState; + importedDataState.appState = stubImportedAppState; + + const restoredAppState = restore.restoreAppState( + importedDataState.appState, + null, + ); + expect(restoredAppState.cursorButton).toBe("up"); + expect(restoredAppState.name).toBe(stubImportedAppState.name); + }); + it("should restore with imported data", () => { const stubImportedAppState = getDefaultAppState(); stubImportedAppState.activeTool.type = "selection"; @@ -638,83 +716,6 @@ describe("restoreAppState", () => { }); }); -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, null); - expect(restoredData.elements.length).toBe(0); - }); - - it("when imported data state is null it should return the local app state property", () => { - const stubLocalAppState = getDefaultAppState(); - stubLocalAppState.cursorButton = "down"; - stubLocalAppState.name = "local app state"; - - const restoredData = restore.restore(null, stubLocalAppState, null); - expect(restoredData.appState.cursorButton).toBe( - stubLocalAppState.cursorButton, - ); - expect(restoredData.appState.name).toBe(stubLocalAppState.name); - }); - - it("when imported data state has elements", () => { - const stubLocalAppState = getDefaultAppState(); - - const textElement = API.createElement({ type: "text" }); - const rectElement = API.createElement({ type: "rectangle" }); - const elements = [textElement, rectElement]; - - const importedDataState = {} as ImportedDataState; - importedDataState.elements = elements; - - const restoredData = restore.restore( - importedDataState, - stubLocalAppState, - null, - ); - expect(restoredData.elements.length).toBe(elements.length); - }); - - it("when local app state is null but imported app state is supplied", () => { - const stubImportedAppState = getDefaultAppState(); - stubImportedAppState.cursorButton = "down"; - stubImportedAppState.name = "imported app state"; - - const importedDataState = {} as ImportedDataState; - importedDataState.appState = stubImportedAppState; - - const restoredData = restore.restore(importedDataState, null, null); - expect(restoredData.appState.cursorButton).toBe("up"); - 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 + 2 }), - expect.objectContaining({ - id: ellipse.id, - version: ellipse.version + 1, - }), - ]); - }); -}); - describe("repairing bindings", () => { it("should repair container boundElements when repair is true", () => { const container = API.createElement({ diff --git a/packages/utils/src/export.ts b/packages/utils/src/export.ts index fefd0db2e0..e29e396300 100644 --- a/packages/utils/src/export.ts +++ b/packages/utils/src/export.ts @@ -7,7 +7,10 @@ import { } from "@excalidraw/excalidraw/clipboard"; import { encodePngMetadata } from "@excalidraw/excalidraw/data/image"; import { serializeAsJSON } from "@excalidraw/excalidraw/data/json"; -import { restore } from "@excalidraw/excalidraw/data/restore"; +import { + restoreAppState, + restoreElements, +} from "@excalidraw/excalidraw/data/restore"; import { exportToCanvas as _exportToCanvas, exportToSvg as _exportToSvg, @@ -45,12 +48,11 @@ export const exportToCanvas = ({ }: ExportOpts & { exportPadding?: number; }) => { - const { elements: restoredElements, appState: restoredAppState } = restore( - { elements, appState }, - null, - null, - { deleteInvisibleElements: true }, - ); + const restoredElements = restoreElements(elements, null, { + deleteInvisibleElements: true, + }); + const restoredAppState = restoreAppState(appState, null); + const { exportBackground, viewBackgroundColor } = restoredAppState; return _exportToCanvas( restoredElements, @@ -176,12 +178,10 @@ export const exportToSvg = async ({ skipInliningFonts?: true; reuseImages?: boolean; }): Promise => { - const { elements: restoredElements, appState: restoredAppState } = restore( - { elements, appState }, - null, - null, - { deleteInvisibleElements: true }, - ); + const restoredElements = restoreElements(elements, null, { + deleteInvisibleElements: true, + }); + const restoredAppState = restoreAppState(appState, null); const exportAppState = { ...restoredAppState,