From 2ffb29dddfbaf960798256a251b6e07bc521826a Mon Sep 17 00:00:00 2001 From: Andrew Kingston Date: Fri, 14 Oct 2022 18:16:19 +0100 Subject: [PATCH] Invert some client store dependencies to improve performance and prevent dependency cycles --- packages/client/manifest.json | 4 ++ .../client/src/components/Component.svelte | 11 +++-- .../src/components/preview/DNDHandler.svelte | 19 ++++---- .../components/preview/HoverIndicator.svelte | 4 +- .../preview/SelectionIndicator.svelte | 4 +- .../src/components/preview/SettingsBar.svelte | 4 +- packages/client/src/index.js | 15 ++----- packages/client/src/stores/builder.js | 8 ---- packages/client/src/stores/components.js | 30 +++++-------- .../src/stores/derived/dndComponentPath.js | 13 ++++++ packages/client/src/stores/derived/index.js | 5 ++- .../client/src/stores/derived/isDragging.js | 10 ----- packages/client/src/stores/dnd.js | 45 ++++++++++++++++--- packages/client/src/stores/index.js | 9 +++- packages/client/src/stores/screens.js | 24 +++++++--- 15 files changed, 122 insertions(+), 83 deletions(-) create mode 100644 packages/client/src/stores/derived/dndComponentPath.js delete mode 100644 packages/client/src/stores/derived/isDragging.js diff --git a/packages/client/manifest.json b/packages/client/manifest.json index 0edb5c0f39..14092b7b4d 100644 --- a/packages/client/manifest.json +++ b/packages/client/manifest.json @@ -85,6 +85,10 @@ "icon": "Selection", "hasChildren": true, "showSettingsBar": true, + "size": { + "width": 400, + "height": 200 + }, "styles": [ "padding", "size", diff --git a/packages/client/src/components/Component.svelte b/packages/client/src/components/Component.svelte index 3d65db7206..fe5f363152 100644 --- a/packages/client/src/components/Component.svelte +++ b/packages/client/src/components/Component.svelte @@ -21,14 +21,14 @@ devToolsStore, componentStore, appStore, - isDragging, + dndIsDragging, + dndComponentPath, } from "stores" import { Helpers } from "@budibase/bbui" import { getActiveConditions, reduceConditionActions } from "utils/conditions" import Placeholder from "components/app/Placeholder.svelte" import ScreenPlaceholder from "components/app/ScreenPlaceholder.svelte" import ComponentPlaceholder from "components/app/ComponentPlaceholder.svelte" - import { DNDPlaceholderID } from "constants" export let instance = {} export let isLayout = false @@ -105,7 +105,7 @@ $builderStore.inBuilder && $builderStore.selectedComponentId === id $: inSelectedPath = $componentStore.selectedComponentPath?.includes(id) $: inDragPath = inSelectedPath && $builderStore.editMode - $: inDndPath = $componentStore.dndPath?.includes(id) + $: inDndPath = $dndComponentPath?.includes(id) // Derive definition properties which can all be optional, so need to be // coerced to booleans @@ -162,7 +162,7 @@ // nested layers. Only reset this when dragging stops. let pad = false $: pad = pad || (interactive && hasChildren && inDndPath) - $: $isDragging, (pad = false) + $: $dndIsDragging, (pad = false) // Update component context $: store.set({ @@ -471,7 +471,6 @@ class:pad class:parent={hasChildren} class:block={isBlock} - class:placeholder={id === DNDPlaceholderID} data-id={id} data-name={name} data-icon={icon} @@ -502,7 +501,7 @@ display: contents; } .component :global(> *) { - transition: padding 260ms ease-in, border 260ms ease-in; + transition: padding 260ms ease-out, border 260ms ease-out; } .component.pad :global(> *) { padding: 12px !important; diff --git a/packages/client/src/components/preview/DNDHandler.svelte b/packages/client/src/components/preview/DNDHandler.svelte index e2222300e0..392cdceb36 100644 --- a/packages/client/src/components/preview/DNDHandler.svelte +++ b/packages/client/src/components/preview/DNDHandler.svelte @@ -4,14 +4,15 @@ import IndicatorSet from "./IndicatorSet.svelte" import { builderStore, - componentStore, + screenStore, dndStore, dndParent, - isDragging, + dndIsDragging, } from "stores" import DNDPlaceholderOverlay from "./DNDPlaceholderOverlay.svelte" import { Utils } from "@budibase/frontend-core" import { findComponentById } from "utils/components.js" + import { DNDPlaceholderID } from "constants" const ThrottleRate = 130 @@ -69,13 +70,13 @@ const id = component.dataset.id const parentId = component.dataset.parent const parent = findComponentById( - get(componentStore).currentAsset.props, + get(screenStore).activeScreen?.props, parentId ) const index = parent._children.findIndex( x => x._id === component.dataset.id ) - dndStore.actions.startDragging({ + dndStore.actions.startDraggingExistingComponent({ id, bounds: component.children[0].getBoundingClientRect(), parent: parentId, @@ -127,7 +128,7 @@ let ephemeralDiv if (node.children.length === 1) { ephemeralDiv = document.createElement("div") - ephemeralDiv.classList.add("placeholder") + ephemeralDiv.dataset.id = DNDPlaceholderID node.appendChild(ephemeralDiv) } @@ -138,7 +139,7 @@ const child = node.children?.[0] || node const bounds = child.getBoundingClientRect() return { - placeholder: node.classList.contains("placeholder"), + placeholder: node.dataset.id === DNDPlaceholderID, centerX: bounds.left + bounds.width / 2, centerY: bounds.top + bounds.height / 2, left: bounds.left, @@ -249,7 +250,7 @@ // Convert parent + index into target + mode let legacyDropTarget, legacyDropMode const parent = findComponentById( - get(componentStore).currentAsset?.props, + get(screenStore).activeScreen?.props, drop.parent ) if (!parent) { @@ -263,7 +264,7 @@ // Filter out source component and placeholder from consideration const children = parent._children?.filter( - x => x._id !== "placeholder" && x._id !== source.id + x => x._id !== DNDPlaceholderID && x._id !== source.id ) // Use inside if no existing children @@ -316,6 +317,6 @@ prefix="Inside" /> -{#if $isDragging} +{#if $dndIsDragging} {/if} diff --git a/packages/client/src/components/preview/HoverIndicator.svelte b/packages/client/src/components/preview/HoverIndicator.svelte index ed28938d4e..d5583ed3db 100644 --- a/packages/client/src/components/preview/HoverIndicator.svelte +++ b/packages/client/src/components/preview/HoverIndicator.svelte @@ -1,7 +1,7 @@ - import { builderStore, isDragging } from "stores" + import { builderStore, dndIsDragging } from "stores" import IndicatorSet from "./IndicatorSet.svelte" $: color = $builderStore.editMode @@ -8,7 +8,7 @@ { diff --git a/packages/client/src/index.js b/packages/client/src/index.js index 383421cc35..706cb4fc9f 100644 --- a/packages/client/src/index.js +++ b/packages/client/src/index.js @@ -64,20 +64,11 @@ const loadBudibase = async () => { } else if (name === "dragging-new-component") { const { dragging, component } = payload if (dragging) { - dndStore.actions.startDragging({ - id: null, - parent: null, - bounds: { - height: 64, - width: 128, - }, - index: null, - newComponentType: component, - }) - builderStore.actions.setDraggingNewComponent(true) + const definition = + componentStore.actions.getComponentDefinition(component) + dndStore.actions.startDraggingNewComponent({ component, definition }) } else { dndStore.actions.reset() - builderStore.actions.setDraggingNewComponent(false) } } } diff --git a/packages/client/src/stores/builder.js b/packages/client/src/stores/builder.js index 135c08343e..b937b7f696 100644 --- a/packages/client/src/stores/builder.js +++ b/packages/client/src/stores/builder.js @@ -16,7 +16,6 @@ const createBuilderStore = () => { theme: null, customTheme: null, previewDevice: "desktop", - draggingNewComponent: false, navigation: null, hiddenComponentIds: [], usedPlugins: null, @@ -68,19 +67,12 @@ const createBuilderStore = () => { }) }, dropNewComponent: (component, parent, index) => { - console.log("dispatch", component, parent, index) dispatchEvent("drop-new-component", { component, parent, index, }) }, - setDraggingNewComponent: draggingNewComponent => { - if (draggingNewComponent === get(store).draggingNewComponent) { - return - } - store.update(state => ({ ...state, draggingNewComponent })) - }, setEditMode: enabled => { if (enabled === get(store).editMode) { return diff --git a/packages/client/src/stores/components.js b/packages/client/src/stores/components.js index e246299741..b34dfe375d 100644 --- a/packages/client/src/stores/components.js +++ b/packages/client/src/stores/components.js @@ -4,7 +4,6 @@ import { findComponentById, findComponentPathById } from "../utils/components" import { devToolsStore } from "./devTools" import { screenStore } from "./screens" import { builderStore } from "./builder" -import { dndParent } from "./dnd.js" import Router from "../components/Router.svelte" import DNDPlaceholder from "../components/preview/DNDPlaceholder.svelte" import * as AppComponents from "../components/app/index.js" @@ -20,28 +19,22 @@ const createComponentStore = () => { }) const derivedStore = derived( - [store, builderStore, devToolsStore, screenStore, dndParent], - ([$store, $builderState, $devToolsState, $screenState, $dndParent]) => { + [store, builderStore, devToolsStore, screenStore], + ([$store, $builderStore, $devToolsStore, $screenStore]) => { + const { inBuilder, selectedComponentId } = $builderStore + // Avoid any of this logic if we aren't in the builder preview - if (!$builderState.inBuilder && !$devToolsState.visible) { + if (!inBuilder && !$devToolsStore.visible) { return {} } - // Derive the selected component instance and definition - let asset - const { screen, selectedComponentId } = $builderState - if ($builderState.inBuilder) { - asset = screen - } else { - asset = $screenState.activeScreen - } - const component = findComponentById(asset?.props, selectedComponentId) + const root = $screenStore.activeScreen?.props + const component = findComponentById(root, selectedComponentId) const definition = getComponentDefinition(component?._component) // Derive the selected component path const selectedPath = - findComponentPathById(asset?.props, selectedComponentId) || [] - const dndPath = findComponentPathById(asset?.props, $dndParent) || [] + findComponentPathById(root, selectedComponentId) || [] return { customComponentManifest: $store.customComponentManifest, @@ -51,9 +44,6 @@ const createComponentStore = () => { selectedComponentDefinition: definition, selectedComponentPath: selectedPath?.map(component => component._id), mountedComponentCount: Object.keys($store.mountedComponents).length, - currentAsset: asset, - dndPath: dndPath?.map(component => component._id), - dndDepth: dndPath?.length || 0, } } ) @@ -101,8 +91,8 @@ const createComponentStore = () => { } const getComponentById = id => { - const asset = get(derivedStore).currentAsset - return findComponentById(asset?.props, id) + const root = get(screenStore).activeScreen?.props + return findComponentById(root, id) } const getComponentDefinition = type => { diff --git a/packages/client/src/stores/derived/dndComponentPath.js b/packages/client/src/stores/derived/dndComponentPath.js new file mode 100644 index 0000000000..58fb395dd6 --- /dev/null +++ b/packages/client/src/stores/derived/dndComponentPath.js @@ -0,0 +1,13 @@ +import { derived } from "svelte/store" +import { findComponentPathById } from "utils/components.js" +import { dndParent } from "../dnd.js" +import { screenStore } from "../screens.js" + +export const dndComponentPath = derived( + [dndParent, screenStore], + ([$dndParent, $screenStore]) => { + const root = $screenStore.activeScreen?.props + const path = findComponentPathById(root, $dndParent) || [] + return path?.map(component => component._id) + } +) diff --git a/packages/client/src/stores/derived/index.js b/packages/client/src/stores/derived/index.js index 85df1aaafa..4f6a6ab91d 100644 --- a/packages/client/src/stores/derived/index.js +++ b/packages/client/src/stores/derived/index.js @@ -1,2 +1,5 @@ -export { isDragging } from "./isDragging.js" +// These derived stores are pulled out from their parent stores to avoid +// dependency loops. By inverting store dependencies and extracting them +// separately we can keep our actual stores lean and performant. export { currentRole } from "./currentRole.js" +export { dndComponentPath } from "./dndComponentPath.js" diff --git a/packages/client/src/stores/derived/isDragging.js b/packages/client/src/stores/derived/isDragging.js deleted file mode 100644 index d3edd586c6..0000000000 --- a/packages/client/src/stores/derived/isDragging.js +++ /dev/null @@ -1,10 +0,0 @@ -import { derived } from "svelte/store" -import { dndStore } from "../dnd" -import { builderStore } from "../builder.js" - -// Derive whether we are dragging or not -export const isDragging = derived( - [dndStore, builderStore], - ([$dndStore, $builderStore]) => - $dndStore.source != null || $builderStore.draggingNewComponent -) diff --git a/packages/client/src/stores/dnd.js b/packages/client/src/stores/dnd.js index 805ec8c339..1e7d7a6d50 100644 --- a/packages/client/src/stores/dnd.js +++ b/packages/client/src/stores/dnd.js @@ -2,6 +2,10 @@ import { writable, derived } from "svelte/store" const createDndStore = () => { const initialState = { + // Flags for whether we are inserting a new component or not + isNewComponent: false, + newComponentType: null, + // Info about the dragged component source: null, @@ -13,12 +17,32 @@ const createDndStore = () => { } const store = writable(initialState) - // newComponentType is an optional field to signify we are creating a new - // component rather than moving an existing one - const startDragging = ({ id, parent, bounds, index, newComponentType }) => { + const startDraggingExistingComponent = ({ id, parent, bounds, index }) => { store.set({ ...initialState, - source: { id, parent, bounds, index, newComponentType }, + source: { id, parent, bounds, index }, + }) + } + + const startDraggingNewComponent = ({ type, definition }) => { + if (!type || !definition) { + return + } + + // Get size of new component so we can show a properly sized placeholder + const width = definition.size?.width || 128 + const height = definition.size?.height || 64 + + store.set({ + ...initialState, + isNewComponent: true, + newComponentType: type, + source: { + id: null, + parent: null, + bounds: { height, width }, + index: null, + }, }) } @@ -43,7 +67,8 @@ const createDndStore = () => { return { subscribe: store.subscribe, actions: { - startDragging, + startDraggingExistingComponent, + startDraggingNewComponent, updateTarget, updateDrop, reset, @@ -52,9 +77,19 @@ const createDndStore = () => { } export const dndStore = createDndStore() + +// The DND store is updated extremely frequently, so we can greatly improve +// performance by deriving any state that needs to be externally observed. +// By doing this and using primitives, we can avoid invalidating other stores +// or components which depend on DND state unless values actually change. +export const dndIsDragging = derived(dndStore, $dndStore => !!$dndStore.source) export const dndParent = derived(dndStore, $dndStore => $dndStore.drop?.parent) export const dndIndex = derived(dndStore, $dndStore => $dndStore.drop?.index) export const dndBounds = derived( dndStore, $dndStore => $dndStore.source?.bounds ) +export const dndIsNewComponent = derived( + dndStore, + $dndStore => $dndStore.isNewComponent +) diff --git a/packages/client/src/stores/index.js b/packages/client/src/stores/index.js index f86c0a0517..c431302d43 100644 --- a/packages/client/src/stores/index.js +++ b/packages/client/src/stores/index.js @@ -15,7 +15,14 @@ export { uploadStore } from "./uploads.js" export { rowSelectionStore } from "./rowSelection.js" export { blockStore } from "./blocks.js" export { environmentStore } from "./environment" -export { dndStore, dndIndex, dndParent, dndBounds } from "./dnd" +export { + dndStore, + dndIndex, + dndParent, + dndBounds, + dndIsNewComponent, + dndIsDragging, +} from "./dnd" // Context stores are layered and duplicated, so it is not a singleton export { createContextStore } from "./context" diff --git a/packages/client/src/stores/screens.js b/packages/client/src/stores/screens.js index b0bc06eee9..0787610d80 100644 --- a/packages/client/src/stores/screens.js +++ b/packages/client/src/stores/screens.js @@ -2,7 +2,7 @@ import { derived } from "svelte/store" import { routeStore } from "./routes" import { builderStore } from "./builder" import { appStore } from "./app" -import { dndIndex, dndParent } from "./dnd.js" +import { dndIndex, dndParent, dndIsNewComponent } from "./dnd.js" import { RoleUtils } from "@budibase/frontend-core" import { findComponentById, findComponentParent } from "../utils/components.js" import { Helpers } from "@budibase/bbui" @@ -10,8 +10,22 @@ import { DNDPlaceholderID, DNDPlaceholderType } from "constants" const createScreenStore = () => { const store = derived( - [appStore, routeStore, builderStore, dndParent, dndIndex], - ([$appStore, $routeStore, $builderStore, $dndParent, $dndIndex]) => { + [ + appStore, + routeStore, + builderStore, + dndParent, + dndIndex, + dndIsNewComponent, + ], + ([ + $appStore, + $routeStore, + $builderStore, + $dndParent, + $dndIndex, + $dndIsNewComponent, + ]) => { let activeLayout, activeScreen let screens @@ -50,8 +64,8 @@ const createScreenStore = () => { if (activeScreen && $dndParent && $dndIndex != null) { // Remove selected component from tree if we are moving an existing // component - const { selectedComponentId, draggingNewComponent } = $builderStore - if (!draggingNewComponent) { + const { selectedComponentId } = $builderStore + if (!$dndIsNewComponent) { let selectedParent = findComponentParent( activeScreen.props, selectedComponentId