From 1172e7200d7c8f5c97291902751084bfcfd05fd3 Mon Sep 17 00:00:00 2001 From: Bogdan Chadkin Date: Sun, 9 Feb 2025 16:11:18 +0700 Subject: [PATCH 1/2] feat: rebind masked variables Here improved creating variables to override deeper bindings when new variable overrides ancestor variable. Here's a video with example --- .../settings-panel/resource-panel.tsx | 8 +- .../settings-panel/variable-popover.tsx | 6 +- .../app/shared/data-variables.test.tsx | 264 ++++++++++++------ apps/builder/app/shared/data-variables.ts | 44 ++- 4 files changed, 217 insertions(+), 105 deletions(-) diff --git a/apps/builder/app/builder/features/settings-panel/resource-panel.tsx b/apps/builder/app/builder/features/settings-panel/resource-panel.tsx index 58bdd5f2ccc8..5f1a5de8679d 100644 --- a/apps/builder/app/builder/features/settings-panel/resource-panel.tsx +++ b/apps/builder/app/builder/features/settings-panel/resource-panel.tsx @@ -59,7 +59,7 @@ import { $selectedPage, } from "~/shared/awareness"; import { updateWebstudioData } from "~/shared/instance-utils"; -import { restoreTreeVariablesMutable } from "~/shared/data-variables"; +import { rebindTreeVariablesMutable } from "~/shared/data-variables"; const validateUrl = (value: string, scope: Record) => { const evaluatedValue = evaluateExpressionWithinScope(value, scope); @@ -608,7 +608,7 @@ export const ResourceForm = forwardRef< updateWebstudioData((data) => { data.dataSources.set(newVariable.id, newVariable); data.resources.set(newResource.id, newResource); - restoreTreeVariablesMutable({ instancePath, ...data }); + rebindTreeVariablesMutable({ instancePath, ...data }); }); }, })); @@ -740,7 +740,7 @@ export const SystemResourceForm = forwardRef< updateWebstudioData((data) => { data.dataSources.set(newVariable.id, newVariable); data.resources.set(newResource.id, newResource); - restoreTreeVariablesMutable({ instancePath, ...data }); + rebindTreeVariablesMutable({ instancePath, ...data }); }); }, })); @@ -856,7 +856,7 @@ export const GraphqlResourceForm = forwardRef< updateWebstudioData((data) => { data.dataSources.set(newVariable.id, newVariable); data.resources.set(newResource.id, newResource); - restoreTreeVariablesMutable({ instancePath, ...data }); + rebindTreeVariablesMutable({ instancePath, ...data }); }); }, })); diff --git a/apps/builder/app/builder/features/settings-panel/variable-popover.tsx b/apps/builder/app/builder/features/settings-panel/variable-popover.tsx index c4eefeba521d..15bce59ddf25 100644 --- a/apps/builder/app/builder/features/settings-panel/variable-popover.tsx +++ b/apps/builder/app/builder/features/settings-panel/variable-popover.tsx @@ -75,7 +75,7 @@ import { generateCurl } from "./curl"; import { updateWebstudioData } from "~/shared/instance-utils"; import { findUnsetVariableNames, - restoreTreeVariablesMutable, + rebindTreeVariablesMutable, } from "~/shared/data-variables"; const $variablesByName = computed( @@ -299,7 +299,7 @@ const ParameterForm = forwardRef< const name = z.string().parse(formData.get("name")); updateWebstudioData((data) => { data.dataSources.set(variable.id, { ...variable, name }); - restoreTreeVariablesMutable({ instancePath, ...data }); + rebindTreeVariablesMutable({ instancePath, ...data }); }); }, })); @@ -339,7 +339,7 @@ const useValuePanelRef = ({ type: "variable", value: variableValue, }); - restoreTreeVariablesMutable({ instancePath, ...data }); + rebindTreeVariablesMutable({ instancePath, ...data }); }); }, })); diff --git a/apps/builder/app/shared/data-variables.test.tsx b/apps/builder/app/shared/data-variables.test.tsx index a335d159e958..096e99dbd296 100644 --- a/apps/builder/app/shared/data-variables.test.tsx +++ b/apps/builder/app/shared/data-variables.test.tsx @@ -1,13 +1,4 @@ import { expect, test, vi } from "vitest"; -import { - computeExpression, - decodeDataVariableName, - encodeDataVariableName, - findUnsetVariableNames, - restoreExpressionVariables, - restoreTreeVariablesMutable, - unsetExpressionVariables, -} from "./data-variables"; import { $, ActionValue, @@ -16,8 +7,17 @@ import { ResourceValue, Variable, } from "@webstudio-is/template"; -import type { InstancePath } from "./awareness"; -import type { Instances } from "@webstudio-is/sdk"; +import { encodeDataVariableId } from "@webstudio-is/sdk"; +import { + computeExpression, + decodeDataVariableName, + encodeDataVariableName, + findUnsetVariableNames, + restoreExpressionVariables, + rebindTreeVariablesMutable, + unsetExpressionVariables, +} from "./data-variables"; +import { getInstancePath } from "./awareness"; test("encode data variable name when necessary", () => { expect(encodeDataVariableName("formState")).toEqual("formState"); @@ -126,24 +126,6 @@ test("compute unset variables as undefined", () => { expect(computeExpression("`${a}`", new Map())).toEqual("undefined"); }); -const getInstancePath = ( - instances: Instances, - instanceSelector: string[] -): InstancePath => { - const instancePath: InstancePath = []; - for (let index = 0; index < instanceSelector.length; index += 1) { - const instanceId = instanceSelector[index]; - const instance = instances.get(instanceId); - if (instance) { - instancePath.push({ - instance, - instanceSelector: instanceSelector.slice(index), - }); - } - } - return instancePath; -}; - test("find unset variable names", () => { const resourceVariable = new ResourceValue("resourceVariable", { url: expression`six`, @@ -171,7 +153,7 @@ test("find unset variable names", () => { ); expect( findUnsetVariableNames({ - instancePath: getInstancePath(data.instances, ["body"]), + instancePath: getInstancePath(["body"], data.instances), ...data, }) ).toEqual([ @@ -189,24 +171,27 @@ test("find unset variable names", () => { }); test("restore tree variables in children", () => { - const oneBody = new Variable("one", "one value of body"); - const oneBox = new Variable("one", "one value of box"); + const bodyVariable = new Variable("one", "one value of body"); + const boxVariable = new Variable("one", "one value of box"); const data = renderData( - <$.Body ws:id="body" data-variables={expression`${oneBody}`}> - <$.Box ws:id="box" data-variables={expression`${oneBox}`}> - <$.Text ws:id="text">{expression`one`} + <$.Body ws:id="bodyId" data-vars={expression`${bodyVariable}`}> + <$.Box ws:id="boxId" data-vars={expression`${boxVariable}`}> + <$.Text ws:id="textId">{expression`one`} ); - restoreTreeVariablesMutable({ - instancePath: getInstancePath(data.instances, ["box", "body"]), + rebindTreeVariablesMutable({ + instancePath: getInstancePath(["boxId", "bodyId"], data.instances), ...data, }); - expect(data.dataSources.get("1")).toEqual( - expect.objectContaining({ name: "one", scopeInstanceId: "box" }) - ); - expect(data.instances.get("text")?.children).toEqual([ - { type: "expression", value: "$ws$dataSource$1" }, + expect(Array.from(data.dataSources.values())).toEqual([ + expect.objectContaining({ scopeInstanceId: "bodyId" }), + expect.objectContaining({ scopeInstanceId: "boxId" }), + ]); + const [_bodyVariableId, boxVariableId] = data.dataSources.keys(); + const boxIdentifier = encodeDataVariableId(boxVariableId); + expect(data.instances.get("textId")?.children).toEqual([ + { type: "expression", value: boxIdentifier }, ]); }); @@ -215,10 +200,10 @@ test("restore tree variables in props", () => { const oneBox = new Variable("one", "one value of box"); const twoBox = new Variable("two", "two value of box"); const data = renderData( - <$.Body ws:id="body" data-variables={expression`${oneBody}`}> + <$.Body ws:id="bodyId" data-body-vars={expression`${oneBody}`}> <$.Box - ws:id="box" - data-variables={expression`${oneBox} ${twoBox}`} + ws:id="boxId" + data-box-vars={expression`${oneBox} ${twoBox}`} data-one={expression`one`} data-action={new ActionValue(["one"], expression`one + two + three`)} > @@ -226,28 +211,83 @@ test("restore tree variables in props", () => { ); - restoreTreeVariablesMutable({ - instancePath: getInstancePath(data.instances, ["box", "body"]), + rebindTreeVariablesMutable({ + instancePath: getInstancePath(["boxId", "bodyId"], data.instances), ...data, }); - expect(data.dataSources.get("1")).toEqual( - expect.objectContaining({ name: "one", scopeInstanceId: "box" }) - ); - expect(data.dataSources.get("2")).toEqual( - expect.objectContaining({ name: "two", scopeInstanceId: "box" }) - ); - expect(data.props.get("box:data-one")?.value).toEqual("$ws$dataSource$1"); - expect(data.props.get("text:data-two")?.value).toEqual( - "$ws$dataSource$1 + $ws$dataSource$2 + three" + const [_bodyVariableId, boxOneVariableId, boxTwoVariableId] = + data.dataSources.keys(); + const boxOneIdentifier = encodeDataVariableId(boxOneVariableId); + const boxTwoIdentifier = encodeDataVariableId(boxTwoVariableId); + expect(Array.from(data.dataSources.values())).toEqual([ + expect.objectContaining({ name: "one", scopeInstanceId: "bodyId" }), + expect.objectContaining({ name: "one", scopeInstanceId: "boxId" }), + expect.objectContaining({ name: "two", scopeInstanceId: "boxId" }), + ]); + expect(Array.from(data.props.values())).toEqual([ + expect.objectContaining({ name: "data-body-vars" }), + expect.objectContaining({ name: "data-box-vars" }), + expect.objectContaining({ name: "data-one", value: boxOneIdentifier }), + expect.objectContaining({ + name: "data-action", + value: [ + { + type: "execute", + args: ["one"], + code: `one + ${boxTwoIdentifier} + three`, + }, + ], + }), + expect.objectContaining({ + name: "data-two", + value: `${boxOneIdentifier} + ${boxTwoIdentifier} + three`, + }), + ]); +}); + +test("rebind tree variables in props and children", () => { + const bodyVariable = new Variable("one", "one value of body"); + const boxVariable = new Variable("one", "one value of box"); + const data = renderData( + <$.Body ws:id="bodyId" data-body-vars={expression`${bodyVariable}`}> + <$.Box ws:id="boxId" data-box-vars={expression`${boxVariable}`}> + <$.Text + ws:id="textId" + data-text-vars={expression`${bodyVariable}`} + data-action={new ActionValue([], expression`${bodyVariable}`)} + > + {expression`${bodyVariable}`} + + + ); - expect(data.props.get("box:data-action")?.value).toEqual([ - { type: "execute", args: ["one"], code: "one + $ws$dataSource$2 + three" }, + rebindTreeVariablesMutable({ + instancePath: getInstancePath(["boxId", "bodyId"], data.instances), + ...data, + }); + expect(Array.from(data.dataSources.values())).toEqual([ + expect.objectContaining({ scopeInstanceId: "bodyId" }), + expect.objectContaining({ scopeInstanceId: "boxId" }), + ]); + const [_bodyVariableId, boxVariableId] = data.dataSources.keys(); + const boxIdentifier = encodeDataVariableId(boxVariableId); + expect(Array.from(data.props.values())).toEqual([ + expect.objectContaining({ name: "data-body-vars" }), + expect.objectContaining({ name: "data-box-vars", value: boxIdentifier }), + expect.objectContaining({ name: "data-text-vars", value: boxIdentifier }), + expect.objectContaining({ + name: "data-action", + value: [{ type: "execute", args: [], code: boxIdentifier }], + }), + ]); + expect(data.instances.get("textId")?.children).toEqual([ + { type: "expression", value: boxIdentifier }, ]); }); test("restore tree variables in resources", () => { - const oneBody = new Variable("one", "one value of body"); - const oneBox = new Variable("one", "one value of box"); + const bodyVariable = new Variable("one", "one value of body"); + const boxVariable = new Variable("one", "one value of box"); const resourceVariable = new ResourceValue("resourceVariable", { url: expression`one + 1`, method: "post", @@ -261,48 +301,92 @@ test("restore tree variables in resources", () => { body: expression`one + 2`, }); const data = renderData( - <$.Body ws:id="body" data-variables={expression`${oneBody}`}> - <$.Box ws:id="box" data-variables={expression`${oneBox}`}> + <$.Body ws:id="bodyId" data-vars={expression`${bodyVariable}`}> + <$.Box ws:id="boxId" data-vars={expression`${boxVariable}`}> <$.Text ws:id="text" - data-variables={expression`${resourceVariable}`} + data-vars={expression`${resourceVariable}`} data-resource={resourceProp} > ); - restoreTreeVariablesMutable({ - instancePath: getInstancePath(data.instances, ["box", "body"]), + rebindTreeVariablesMutable({ + instancePath: getInstancePath(["boxId", "bodyId"], data.instances), ...data, }); - expect(data.dataSources.get("1")).toEqual( - expect.objectContaining({ name: "one", scopeInstanceId: "box" }) - ); - expect(data.props.get("text:data-variables")?.value).toEqual( - `$ws$dataSource$2` - ); - expect(data.dataSources.get("2")).toEqual( + expect(Array.from(data.dataSources.values())).toEqual([ + expect.objectContaining({ scopeInstanceId: "bodyId" }), + expect.objectContaining({ scopeInstanceId: "boxId" }), + expect.objectContaining({ type: "resource" }), + ]); + const [_bodyVariableId, boxVariableId] = data.dataSources.keys(); + const boxIdentifier = encodeDataVariableId(boxVariableId); + expect(Array.from(data.resources.values())).toEqual([ expect.objectContaining({ - name: "resourceVariable", - scopeInstanceId: "text", - resourceId: "resource:2", - }) - ); - expect(data.resources.get("resource:2")).toEqual({ - id: "resource:2", - name: "resourceVariable", - url: "$ws$dataSource$1 + 1", + url: `${boxIdentifier} + 1`, + method: "post", + headers: [{ name: "auth", value: `${boxIdentifier} + 1` }], + body: `${boxIdentifier} + 1`, + }), + expect.objectContaining({ + url: `${boxIdentifier} + 2`, + method: "post", + headers: [{ name: "auth", value: `${boxIdentifier} + 2` }], + body: `${boxIdentifier} + 2`, + }), + ]); +}); + +test("rebind tree variables in resources", () => { + const bodyVariable = new Variable("one", "one value of body"); + const boxVariable = new Variable("one", "one value of box"); + const resourceVariable = new ResourceValue("resourceVariable", { + url: expression`${bodyVariable}`, method: "post", - headers: [{ name: "auth", value: "$ws$dataSource$1 + 1" }], - body: "$ws$dataSource$1 + 1", + headers: [{ name: "auth", value: expression`${bodyVariable}` }], + body: expression`${bodyVariable}`, }); - expect(data.props.get("text:data-resource")?.value).toEqual(`resource:3`); - expect(data.resources.get("resource:3")).toEqual({ - id: "resource:3", - name: "resourceProp", - url: "$ws$dataSource$1 + 2", + const resourceProp = new ResourceValue("resourceProp", { + url: expression`${bodyVariable}`, method: "post", - headers: [{ name: "auth", value: "$ws$dataSource$1 + 2" }], - body: "$ws$dataSource$1 + 2", + headers: [{ name: "auth", value: expression`${bodyVariable}` }], + body: expression`${bodyVariable}`, + }); + const data = renderData( + <$.Body ws:id="bodyId" data-body-vars={expression`${bodyVariable}`}> + <$.Box ws:id="boxId" data-box-vars={expression`${boxVariable}`}> + <$.Text + ws:id="textId" + data-text-vars={expression`${resourceVariable}`} + data-action={resourceProp} + > + + + ); + rebindTreeVariablesMutable({ + instancePath: getInstancePath(["boxId", "bodyId"], data.instances), + ...data, }); + expect(Array.from(data.dataSources.values())).toEqual([ + expect.objectContaining({ scopeInstanceId: "bodyId" }), + expect.objectContaining({ scopeInstanceId: "boxId" }), + expect.objectContaining({ type: "resource" }), + ]); + const [_bodyVariableId, boxVariableId] = data.dataSources.keys(); + const boxIdentifier = encodeDataVariableId(boxVariableId); + expect(Array.from(data.resources.values())).toEqual([ + expect.objectContaining({ + url: boxIdentifier, + method: "post", + headers: [{ name: "auth", value: boxIdentifier }], + body: boxIdentifier, + }), + expect.objectContaining({ + url: boxIdentifier, + method: "post", + headers: [{ name: "auth", value: boxIdentifier }], + body: boxIdentifier, + }), + ]); }); diff --git a/apps/builder/app/shared/data-variables.ts b/apps/builder/app/shared/data-variables.ts index 82fe73db2032..b9449e779433 100644 --- a/apps/builder/app/shared/data-variables.ts +++ b/apps/builder/app/shared/data-variables.ts @@ -292,7 +292,7 @@ export const findUnsetVariableNames = ({ return Array.from(unsetVariables); }; -export const restoreTreeVariablesMutable = ({ +export const rebindTreeVariablesMutable = ({ instancePath, instances, props, @@ -305,7 +305,11 @@ export const restoreTreeVariablesMutable = ({ dataSources: DataSources; resources: Resources; }) => { - const maskedVariables = findMaskedVariables({ instancePath, dataSources }); + const maskedIdByName = findMaskedVariables({ instancePath, dataSources }); + const unsetNameById = new Map(); + for (const { id, name } of dataSources.values()) { + unsetNameById.set(id, name); + } const [{ instance: startingInstance }] = instancePath; const instanceIds = findTreeInstanceIdsExcludingSlotDescendants( instances, @@ -319,9 +323,13 @@ export const restoreTreeVariablesMutable = ({ } for (const child of instance.children) { if (child.type === "expression") { + child.value = unsetExpressionVariables({ + expression: child.value, + unsetNameById, + }); child.value = restoreExpressionVariables({ expression: child.value, - maskedIdByName: maskedVariables, + maskedIdByName, }); } } @@ -332,18 +340,26 @@ export const restoreTreeVariablesMutable = ({ continue; } if (prop.type === "expression") { + prop.value = unsetExpressionVariables({ + expression: prop.value, + unsetNameById, + }); prop.value = restoreExpressionVariables({ expression: prop.value, - maskedIdByName: maskedVariables, + maskedIdByName, }); continue; } if (prop.type === "action") { for (const action of prop.value) { - const maskedVariablesWithoutArgs = new Map(maskedVariables); + const maskedVariablesWithoutArgs = new Map(maskedIdByName); for (const arg of action.args) { maskedVariablesWithoutArgs.delete(arg); } + action.code = unsetExpressionVariables({ + expression: action.code, + unsetNameById, + }); action.code = restoreExpressionVariables({ expression: action.code, maskedIdByName: maskedVariablesWithoutArgs, @@ -370,20 +386,32 @@ export const restoreTreeVariablesMutable = ({ if (resourceIds.has(resource.id) === false) { continue; } + resource.url = unsetExpressionVariables({ + expression: resource.url, + unsetNameById, + }); resource.url = restoreExpressionVariables({ expression: resource.url, - maskedIdByName: maskedVariables, + maskedIdByName, }); for (const header of resource.headers) { + header.value = unsetExpressionVariables({ + expression: header.value, + unsetNameById, + }); header.value = restoreExpressionVariables({ expression: header.value, - maskedIdByName: maskedVariables, + maskedIdByName, }); } if (resource.body) { + resource.body = unsetExpressionVariables({ + expression: resource.body, + unsetNameById, + }); resource.body = restoreExpressionVariables({ expression: resource.body, - maskedIdByName: maskedVariables, + maskedIdByName, }); } } From 5fd0e6d0eb48822715bc47d17c96049243548235 Mon Sep 17 00:00:00 2001 From: Bogdan Chadkin Date: Sun, 9 Feb 2025 22:11:46 +0700 Subject: [PATCH 2/2] Insert name identifier instead of encoded id in bindings --- apps/builder/app/builder/shared/binding-popover.tsx | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/apps/builder/app/builder/shared/binding-popover.tsx b/apps/builder/app/builder/shared/binding-popover.tsx index f99df7eff38c..0dcb08ad402f 100644 --- a/apps/builder/app/builder/shared/binding-popover.tsx +++ b/apps/builder/app/builder/shared/binding-popover.tsx @@ -39,7 +39,10 @@ import { lintExpression, } from "@webstudio-is/sdk"; import { $dataSourceVariables, $isDesignMode } from "~/shared/nano-states"; -import { computeExpression } from "~/shared/data-variables"; +import { + computeExpression, + encodeDataVariableName, +} from "~/shared/data-variables"; import { ExpressionEditor, formatValuePreview, @@ -146,7 +149,10 @@ const BindingPanel = ({ active={usedIdentifiers.has(identifier)} // convert variable to expression onClick={() => { - editorApiRef.current?.replaceSelection(identifier); + if (name) { + const nameIdentifier = encodeDataVariableName(name); + editorApiRef.current?.replaceSelection(nameIdentifier); + } }} // expression editor blur is fired after pointer down even // preventing it allows to not trigger validation