Skip to content

Commit

Permalink
feat: allow saving expressions with unset variables (#4792)
Browse files Browse the repository at this point in the history
Ref #4768
#4753

User can paste expression with not defined variable and should get it
saved in case of accidental closing popover.

Now such expressions can be executed in builder and will be compiled
with undefined instead of variables in published sites.

In the future we need to add global diagnostics to let users find such
places faster.

Also fixed a couple of ui issues
- react error about using button inside of button in data variables list
- react error about using unknown middlewareData prop from floating-ui

<img width="307" alt="Screenshot 2025-01-27 at 16 30 58"
src="https://github.com/user-attachments/assets/2ddc9ead-33a2-4438-b641-6943d7472f7e"
/>
  • Loading branch information
TrySound authored Jan 27, 2025
1 parent 0586f6e commit 6b04c97
Show file tree
Hide file tree
Showing 15 changed files with 240 additions and 143 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,11 @@ import {
import { TrashIcon, PlusIcon } from "@webstudio-is/icons";
import { isFeatureEnabled } from "@webstudio-is/feature-flags";
import { isLiteralExpression } from "@webstudio-is/sdk";
import { computeExpression } from "~/shared/data-variables";
import {
BindingControl,
BindingPopover,
} from "~/builder/shared/binding-popover";
import { computeExpression } from "~/shared/nano-states";
import { $pageRootScope } from "./page-utils";

type Meta = {
Expand Down
2 changes: 1 addition & 1 deletion apps/builder/app/builder/features/pages/page-settings.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,6 @@ import {
$instances,
$pages,
$dataSources,
computeExpression,
$dataSourceVariables,
$publishedOrigin,
$project,
Expand Down Expand Up @@ -112,6 +111,7 @@ import type { UserPlanFeatures } from "~/shared/db/user-plan-features.server";
import { useUnmount } from "~/shared/hook-utils/use-mount";
import { Card } from "../marketplace/card";
import { selectInstance } from "~/shared/awareness";
import { computeExpression } from "~/shared/data-variables";

const fieldDefaultValues = {
name: "Untitled",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -442,6 +442,7 @@ BooleanForm.displayName = "BooleanForm";

const validateJsonValue = (expression: string) => {
const diagnostics = lintExpression({ expression });
// prevent saving with any message including unset variable
return diagnostics.length > 0 ? "error" : "";
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -220,8 +220,10 @@ const VariablesItem = ({
id={variable.id}
index={index}
label={
<Flex align="center" css={{}}>
<Label color={source}>{variable.name}</Label>
<Flex align="center">
<Label tag="label" color={source}>
{variable.name}
</Label>
{value !== undefined && (
<span className={variableLabelStyle.toString()}>
&nbsp;
Expand Down
11 changes: 5 additions & 6 deletions apps/builder/app/builder/shared/binding-popover.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -38,16 +38,13 @@ import {
getExpressionIdentifiers,
lintExpression,
} from "@webstudio-is/sdk";
import { $dataSourceVariables, $isDesignMode } from "~/shared/nano-states";
import { computeExpression } from "~/shared/data-variables";
import {
ExpressionEditor,
formatValuePreview,
type EditorApi,
} from "./expression-editor";
import {
$dataSourceVariables,
$isDesignMode,
computeExpression,
} from "~/shared/nano-states";

export const evaluateExpressionWithinScope = (
expression: string,
Expand Down Expand Up @@ -94,7 +91,9 @@ const BindingPanel = ({
expression,
availableVariables: new Set(aliases.keys()),
});
setErrorsCount(diagnostics.length);
// prevent saving expression only with syntax error
const errors = diagnostics.filter((item) => item.severity === "error");
setErrorsCount(errors.length);
};

const updateExpression = (newExpression: string) => {
Expand Down
4 changes: 4 additions & 0 deletions apps/builder/app/builder/shared/code-editor-base.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,10 @@ const editorContentStyle = css({
textDecoration: "underline wavy red",
backgroundColor: "rgba(255, 0, 0, 0.1)",
},
".cm-lintRange-warning": {
textDecoration: "underline wavy orange",
backgroundColor: "rgba(255, 0, 0, 0.1)",
},
".cm-gutters": {
backgroundColor: "transparent",
border: 0,
Expand Down
59 changes: 58 additions & 1 deletion apps/builder/app/shared/data-variables.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { expect, test } from "vitest";
import { expect, test, vi } from "vitest";
import {
computeExpression,
decodeDataVariableName,
encodeDataVariableName,
restoreExpressionVariables,
Expand Down Expand Up @@ -56,3 +57,59 @@ test("restore expression variables", () => {
})
).toEqual("$ws$dataSource$myId + missingVariable");
});

test("compute expression with decoded ids", () => {
expect(
computeExpression("$ws$dataSource$myId", new Map([["myId", "value"]]))
).toEqual("value");
});

test("compute expression with decoded names", () => {
expect(
computeExpression("My$32$Name", new Map([["My Name", "value"]]))
).toEqual("value");
});

test("compute expression when invalid syntax", () => {
// prevent error message in test report
const spy = vi.spyOn(console, "error");
spy.mockImplementationOnce(() => {});
expect(computeExpression("https://github.com", new Map())).toEqual(undefined);
expect(spy).toHaveBeenCalledOnce();
spy.mockRestore();
});

test("compute expression with nested field of undefined without error", () => {
const spy = vi.spyOn(console, "error");
const variables = new Map([["myVariable", undefined]]);
expect(computeExpression("myVariable.field", variables)).toEqual(undefined);
expect(spy).not.toHaveBeenCalled();
spy.mockRestore();
});

test("compute literal expression when variable is json object", () => {
const jsonObject = { hello: "world", subObject: { world: "hello" } };
const variables = new Map([["jsonVariable", jsonObject]]);
expect(computeExpression("`${jsonVariable}`", variables)).toEqual(
`{"hello":"world","subObject":{"world":"hello"}}`
);
expect(computeExpression("`${jsonVariable.subObject}`", variables)).toEqual(
`{"world":"hello"}`
);
});

test("compute literal expression when object is frozen", () => {
const jsonObject = Object.freeze({
hello: "world",
subObject: { world: "hello" },
});
const variables = new Map([["jsonVariable", jsonObject]]);
expect(computeExpression("`${jsonVariable.subObject}`", variables)).toEqual(
`{"world":"hello"}`
);
});

test("compute unset variables as undefined", () => {
expect(computeExpression(`a`, new Map())).toEqual(undefined);
expect(computeExpression("`${a}`", new Map())).toEqual("undefined");
});
81 changes: 81 additions & 0 deletions apps/builder/app/shared/data-variables.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,10 @@ import {
encodeDataVariableId,
transpileExpression,
} from "@webstudio-is/sdk";
import {
createJsonStringifyProxy,
isPlainObject,
} from "@webstudio-is/sdk/runtime";

const allowedJsChars = /[A-Za-z_]/;

Expand Down Expand Up @@ -92,3 +96,80 @@ export const restoreExpressionVariables = ({
return expression;
}
};

export const computeExpression = (
expression: string,
variables: Map<DataSource["name"], unknown>
) => {
try {
const usedVariables = new Map();
const transpiled = transpileExpression({
expression,
executable: true,
replaceVariable: (identifier) => {
const id = decodeDataVariableId(identifier);
if (id) {
usedVariables.set(identifier, id);
} else {
// access all variable values from specified map
const name = decodeDataVariableName(identifier);
usedVariables.set(identifier, name);
}
},
});
let code = "";
// add only used variables in expression and get values
// from variables map without additional serializing of these values
for (const [identifier, name] of usedVariables) {
code += `let ${identifier} = _variables.get(${JSON.stringify(name)});\n`;
}
code += `return (${transpiled})`;

/**
*
* We are using structuredClone on frozen values because, for some reason,
* the Proxy example below throws a cryptic error:
* TypeError: 'get' on proxy: property 'data' is a read-only and non-configurable
* data property on the proxy target, but the proxy did not return its actual value
* (expected '[object Array]' but got '[object Array]').
*
* ```
* const createJsonStringifyProxy = (target) => {
* return new Proxy(target, {
* get(target, prop, receiver) {
*
* console.log((prop in target), prop)
*
* const value = Reflect.get(target, prop, receiver);
*
* if (typeof value === "object" && value !== null) {
* return createJsonStringifyProxy(value);
* }
*
* return value;
* },
* });
* };
* const obj = Object.freeze({ data: [1, 2, 3, 4] });
* const proxy = createJsonStringifyProxy(obj)
* proxy.data
*
* ```
*/
const proxiedVariables = new Map(
[...variables.entries()].map(([key, value]) => [
key,
isPlainObject(value)
? createJsonStringifyProxy(
Object.isFrozen(value) ? structuredClone(value) : value
)
: value,
])
);

const result = new Function("_variables", code)(proxiedVariables);
return result;
} catch (error) {
console.error(error);
}
};
28 changes: 1 addition & 27 deletions apps/builder/app/shared/nano-states/props.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,12 @@ import { beforeEach, expect, test } from "vitest";
import { cleanStores } from "nanostores";
import { createDefaultPages } from "@webstudio-is/project-build";
import { setEnv } from "@webstudio-is/feature-flags";
import {
type Instance,
encodeDataSourceVariable,
collectionComponent,
} from "@webstudio-is/sdk";
import { type Instance, collectionComponent } from "@webstudio-is/sdk";
import { textContentAttribute } from "@webstudio-is/react-sdk";
import { $instances } from "./instances";
import {
$propValuesByInstanceSelector,
$variableValuesByInstanceSelector,
computeExpression,
} from "./props";
import { $pages } from "./pages";
import { $assets, $dataSources, $props, $resources } from "./misc";
Expand Down Expand Up @@ -978,24 +973,3 @@ test("prefill default system variable value", () => {

cleanStores($variableValuesByInstanceSelector);
});

test("compute expression when invalid syntax", () => {
expect(computeExpression("https://github.com", new Map())).toEqual(undefined);
});

test("compute literal expression when variable is json object", () => {
const variableName = "jsonVariable";
const encVariableName = encodeDataSourceVariable(variableName);
const jsonObject = { hello: "world", subObject: { world: "hello" } };
const variables = new Map([[variableName, jsonObject]]);
const expression = "`${" + encVariableName + "}`";

expect(computeExpression(expression, variables)).toEqual(
JSON.stringify(jsonObject)
);

const subObjectExpression = "`${" + encVariableName + ".subObject}`";
expect(computeExpression(subObjectExpression, variables)).toEqual(
JSON.stringify(jsonObject.subObject)
);
});
78 changes: 1 addition & 77 deletions apps/builder/app/shared/nano-states/props.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,6 @@ import {
collectionComponent,
portalComponent,
} from "@webstudio-is/sdk";
import {
createJsonStringifyProxy,
isPlainObject,
} from "@webstudio-is/sdk/runtime";
import { normalizeProps, textContentAttribute } from "@webstudio-is/react-sdk";
import { isFeatureEnabled } from "@webstudio-is/feature-flags";
import { mapGroupBy } from "~/shared/shim";
Expand All @@ -44,6 +40,7 @@ import {
import { uploadingFileDataToAsset } from "~/builder/shared/assets/asset-utils";
import { fetch } from "~/shared/fetch.client";
import { $selectedPage, getInstanceKey } from "../awareness";
import { computeExpression } from "../data-variables";

export const assetBaseUrl = "/cgi/asset/";

Expand Down Expand Up @@ -204,79 +201,6 @@ const $loaderVariableValues = computed(
}
);

export const computeExpression = (
expression: string,
variables: Map<DataSource["id"], unknown>
) => {
try {
const usedVariables = new Map();
const transpiled = transpileExpression({
expression,
executable: true,
replaceVariable: (identifier) => {
const id = decodeDataSourceVariable(identifier);
if (id) {
usedVariables.set(identifier, id);
}
},
});
let code = "";
// add only used variables in expression and get values
// from variables map without additional serializing of these values
for (const [identifier, id] of usedVariables) {
code += `let ${identifier} = _variables.get("${id}");\n`;
}
code += `return (${transpiled})`;

/**
*
* We are using structuredClone on frozen values because, for some reason,
* the Proxy example below throws a cryptic error:
* TypeError: 'get' on proxy: property 'data' is a read-only and non-configurable
* data property on the proxy target, but the proxy did not return its actual value
* (expected '[object Array]' but got '[object Array]').
*
* ```
* const createJsonStringifyProxy = (target) => {
* return new Proxy(target, {
* get(target, prop, receiver) {
*
* console.log((prop in target), prop)
*
* const value = Reflect.get(target, prop, receiver);
*
* if (typeof value === "object" && value !== null) {
* return createJsonStringifyProxy(value);
* }
*
* return value;
* },
* });
* };
* const obj = Object.freeze({ data: [1, 2, 3, 4] });
* const proxy = createJsonStringifyProxy(obj)
* proxy.data
*
* ```
*/
const proxiedVariables = new Map(
[...variables.entries()].map(([key, value]) => [
key,
isPlainObject(value)
? createJsonStringifyProxy(
Object.isFrozen(value) ? structuredClone(value) : value
)
: value,
])
);

const result = new Function("_variables", code)(proxiedVariables);
return result;
} catch (error) {
console.error(error);
}
};

/**
* compute prop values within context of instance ancestors
* like a dry-run of rendering and accessing react contexts deep in the tree
Expand Down
Loading

0 comments on commit 6b04c97

Please sign in to comment.