From 7618ca48d76e582d1bcef1bea8d25577510bd8af Mon Sep 17 00:00:00 2001 From: David Luzar Date: Tue, 13 Oct 2020 13:46:52 +0200 Subject: [PATCH] retain local appState props on restore (#2224) Co-authored-by: Lipis --- src/components/App.tsx | 8 +++++-- src/data/blob.ts | 31 ++++++++++++++----------- src/data/index.ts | 30 +++++++++++++----------- src/data/json.ts | 4 ++-- src/data/localStorage.ts | 6 ++--- src/data/restore.ts | 38 ++++++++++++++++++++++-------- src/tests/appState.test.tsx | 46 +++++++++++++++++++++++++++++++++++++ src/tests/helpers/api.ts | 26 +++++++++++++++++++++ src/tests/history.test.tsx | 33 +++++++------------------- 9 files changed, 153 insertions(+), 69 deletions(-) create mode 100644 src/tests/appState.test.tsx diff --git a/src/components/App.tsx b/src/components/App.tsx index 32741ec8..13298f0a 100644 --- a/src/components/App.tsx +++ b/src/components/App.tsx @@ -599,9 +599,13 @@ class App extends React.Component { ) { // Backwards compatibility with legacy url format if (id) { - scene = await loadScene(id); + scene = await loadScene(id, null, this.props.initialData); } else if (jsonMatch) { - scene = await loadScene(jsonMatch[1], jsonMatch[2]); + scene = await loadScene( + jsonMatch[1], + jsonMatch[2], + this.props.initialData, + ); } if (!isCollaborationScene) { window.history.replaceState({}, "Excalidraw", window.location.origin); diff --git a/src/data/blob.ts b/src/data/blob.ts index d83eb1ad..e39a39f4 100644 --- a/src/data/blob.ts +++ b/src/data/blob.ts @@ -23,11 +23,11 @@ const loadFileContents = async (blob: any) => { return contents; }; -/** - * @param blob - * @param appState if provided, used for centering scroll to restored scene - */ -export const loadFromBlob = async (blob: any, appState?: AppState) => { +export const loadFromBlob = async ( + blob: any, + /** @see restore.localAppState */ + localAppState: AppState | null, +) => { if (blob.handle) { // TODO: Make this part of `AppState`. (window as any).handle = blob.handle; @@ -39,16 +39,19 @@ export const loadFromBlob = async (blob: any, appState?: AppState) => { if (data.type !== "excalidraw") { throw new Error(t("alerts.couldNotLoadInvalidFile")); } - return restore({ - elements: data.elements, - appState: { - appearance: appState?.appearance, - ...cleanAppStateForExport(data.appState || {}), - ...(appState - ? calculateScrollCenter(data.elements || [], appState, null) - : {}), + return restore( + { + elements: data.elements, + appState: { + appearance: localAppState?.appearance, + ...cleanAppStateForExport(data.appState || {}), + ...(localAppState + ? calculateScrollCenter(data.elements || [], localAppState, null) + : {}), + }, }, - }); + localAppState, + ); } catch { throw new Error(t("alerts.couldNotLoadInvalidFile")); } diff --git a/src/data/index.ts b/src/data/index.ts index ba2307fe..8ce3d34f 100644 --- a/src/data/index.ts +++ b/src/data/index.ts @@ -233,18 +233,15 @@ const importFromBackend = async ( id: string | null, privateKey?: string | null, ): Promise => { - let elements: readonly ExcalidrawElement[] = []; - let appState = getDefaultAppState(); - try { const response = await fetch( privateKey ? `${BACKEND_V2_GET}${id}` : `${BACKEND_GET}${id}.json`, ); if (!response.ok) { window.alert(t("alerts.importBackendFailed")); - return { elements, appState }; + return {}; } - let data; + let data: ImportedDataState; if (privateKey) { const buffer = await response.arrayBuffer(); const key = await getImportedKey(privateKey, "decrypt"); @@ -267,13 +264,14 @@ const importFromBackend = async ( data = await response.json(); } - elements = data.elements || elements; - appState = { ...appState, ...data.appState }; + return { + elements: data.elements || null, + appState: data.appState || null, + }; } catch (error) { window.alert(t("alerts.importBackendFailed")); console.error(error); - } finally { - return { elements, appState }; + return {}; } }; @@ -363,16 +361,22 @@ export const exportCanvas = async ( export const loadScene = async ( id: string | null, - privateKey?: string | null, - initialData?: ImportedDataState, + privateKey: string | null, + // Supply initialData 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`. + initialData: ImportedDataState | undefined | null, ) => { let data; if (id != 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)); + data = restore( + await importFromBackend(id, privateKey), + initialData?.appState, + ); } else { - data = restore(initialData || {}); + data = restore(initialData || {}, null); } return { diff --git a/src/data/json.ts b/src/data/json.ts index 7f56c0d3..84b47614 100644 --- a/src/data/json.ts +++ b/src/data/json.ts @@ -45,13 +45,13 @@ export const saveAsJSON = async ( ); }; -export const loadFromJSON = async (appState: AppState) => { +export const loadFromJSON = async (localAppState: AppState) => { const blob = await fileOpen({ description: "Excalidraw files", extensions: [".json", ".excalidraw"], mimeTypes: ["application/json"], }); - return loadFromBlob(blob, appState); + return loadFromBlob(blob, localAppState); }; export const isValidLibrary = (json: any) => { diff --git a/src/data/localStorage.ts b/src/data/localStorage.ts index ec5f60ed..0f43578b 100644 --- a/src/data/localStorage.ts +++ b/src/data/localStorage.ts @@ -1,7 +1,7 @@ import { ExcalidrawElement } from "../element/types"; import { AppState, LibraryItems } from "../types"; import { clearAppStateForLocalStorage, getDefaultAppState } from "../appState"; -import { restore } from "./restore"; +import { restoreElements } from "./restore"; const LOCAL_STORAGE_KEY = "excalidraw"; const LOCAL_STORAGE_KEY_STATE = "excalidraw-state"; @@ -21,8 +21,8 @@ export const loadLibrary = (): Promise => { return resolve([]); } - const items = (JSON.parse(data) as LibraryItems).map( - (elements) => restore({ elements, appState: null }).elements, + const items = (JSON.parse(data) as LibraryItems).map((elements) => + restoreElements(elements), ) as Mutable; // clone to ensure we don't mutate the cached library elements in the app diff --git a/src/data/restore.ts b/src/data/restore.ts index 60723030..9d352746 100644 --- a/src/data/restore.ts +++ b/src/data/restore.ts @@ -118,7 +118,7 @@ const restoreElement = ( } }; -const restoreElements = ( +export const restoreElements = ( elements: ImportedDataState["elements"], ): ExcalidrawElement[] => { return (elements || []).reduce((elements, element) => { @@ -134,18 +134,27 @@ const restoreElements = ( }, [] as ExcalidrawElement[]); }; -const restoreAppState = (appState: ImportedDataState["appState"]): AppState => { +const restoreAppState = ( + appState: ImportedDataState["appState"], + localAppState: Partial | null, +): AppState => { appState = appState || {}; const defaultAppState = getDefaultAppState(); const nextAppState = {} as typeof defaultAppState; - for (const [key, val] of Object.entries(defaultAppState)) { - if ((appState as any)[key] !== undefined) { - (nextAppState as any)[key] = (appState as any)[key]; - } else { - (nextAppState as any)[key] = val; - } + for (const [key, val] of Object.entries(defaultAppState) as [ + keyof typeof defaultAppState, + any, + ][]) { + const restoredValue = appState[key]; + const localValue = localAppState ? localAppState[key] : undefined; + (nextAppState as any)[key] = + restoredValue !== undefined + ? restoredValue + : localValue !== undefined + ? localValue + : val; } return { @@ -155,9 +164,18 @@ const restoreAppState = (appState: ImportedDataState["appState"]): AppState => { }; }; -export const restore = (data: ImportedDataState): DataState => { +export const restore = ( + data: ImportedDataState, + /** + * 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, +): DataState => { return { elements: restoreElements(data.elements), - appState: restoreAppState(data.appState), + appState: restoreAppState(data.appState, localAppState || null), }; }; diff --git a/src/tests/appState.test.tsx b/src/tests/appState.test.tsx new file mode 100644 index 00000000..6f8a304f --- /dev/null +++ b/src/tests/appState.test.tsx @@ -0,0 +1,46 @@ +import React from "react"; +import { render, waitFor } from "./test-utils"; +import App from "../components/App"; +import { API } from "./helpers/api"; +import { getDefaultAppState } from "../appState"; + +const { h } = window; + +describe("appState", () => { + it("drag&drop file doesn't reset non-persisted appState", async () => { + const defaultAppState = getDefaultAppState(); + const exportBackground = !defaultAppState.exportBackground; + render( + , + ); + + await waitFor(() => { + expect(h.state.exportBackground).toBe(exportBackground); + expect(h.state.viewBackgroundColor).toBe("#F00"); + }); + + API.dropFile({ + appState: { + viewBackgroundColor: "#000", + }, + elements: [API.createElement({ type: "rectangle", id: "A" })], + }); + + await waitFor(() => { + expect(h.elements).toEqual([expect.objectContaining({ id: "A" })]); + // non-imported prop → retain + expect(h.state.exportBackground).toBe(exportBackground); + // imported prop → overwrite + expect(h.state.viewBackgroundColor).toBe("#000"); + }); + }); +}); diff --git a/src/tests/helpers/api.ts b/src/tests/helpers/api.ts index 9c39377c..5a2e82c6 100644 --- a/src/tests/helpers/api.ts +++ b/src/tests/helpers/api.ts @@ -7,6 +7,8 @@ import { import { newElement, newTextElement, newLinearElement } from "../../element"; import { DEFAULT_VERTICAL_ALIGN } from "../../constants"; import { getDefaultAppState } from "../../appState"; +import { GlobalTestState, createEvent, fireEvent } from "../test-utils"; +import { ImportedDataState } from "../../data/types"; const { h } = window; @@ -135,4 +137,28 @@ export class API { } return element as any; }; + + static dropFile(sceneData: ImportedDataState) { + const fileDropEvent = createEvent.drop(GlobalTestState.canvas); + const file = new Blob( + [ + JSON.stringify({ + type: "excalidraw", + ...sceneData, + }), + ], + { + type: "application/json", + }, + ); + Object.defineProperty(fileDropEvent, "dataTransfer", { + value: { + files: [file], + getData: (_type: string) => { + return ""; + }, + }, + }); + fireEvent(GlobalTestState.canvas, fileDropEvent); + } } diff --git a/src/tests/history.test.tsx b/src/tests/history.test.tsx index 7a47fb09..fc6dd41b 100644 --- a/src/tests/history.test.tsx +++ b/src/tests/history.test.tsx @@ -1,10 +1,10 @@ import React from "react"; -import { render, GlobalTestState } from "./test-utils"; +import { render } from "./test-utils"; import App from "../components/App"; import { UI } from "./helpers/ui"; import { API } from "./helpers/api"; import { getDefaultAppState } from "../appState"; -import { waitFor, fireEvent, createEvent } from "@testing-library/react"; +import { waitFor } from "@testing-library/react"; import { createUndoAction, createRedoAction } from "../actions/actionHistory"; const { h } = window; @@ -77,31 +77,14 @@ describe("history", () => { await waitFor(() => expect(h.elements).toEqual([expect.objectContaining({ id: "A" })]), ); - const fileDropEvent = createEvent.drop(GlobalTestState.canvas); - const file = new Blob( - [ - JSON.stringify({ - type: "excalidraw", - appState: { - ...getDefaultAppState(), - viewBackgroundColor: "#000", - }, - elements: [API.createElement({ type: "rectangle", id: "B" })], - }), - ], - { - type: "application/json", - }, - ); - Object.defineProperty(fileDropEvent, "dataTransfer", { - value: { - files: [file], - getData: (_type: string) => { - return ""; - }, + + API.dropFile({ + appState: { + ...getDefaultAppState(), + viewBackgroundColor: "#000", }, + elements: [API.createElement({ type: "rectangle", id: "B" })], }); - fireEvent(GlobalTestState.canvas, fileDropEvent); await waitFor(() => expect(API.getStateHistory().length).toBe(2)); expect(h.state.viewBackgroundColor).toBe("#000");