-
-
Notifications
You must be signed in to change notification settings - Fork 50
Add code input for number props #364
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| /** | ||
| * Copyright (c) 2022—present Michael Dougall. All rights reserved. | ||
| * | ||
| * This repository utilizes multiple licenses across different directories. To | ||
| * see this files license find the nearest LICENSE file up the source tree. | ||
| */ | ||
| import { memo } from "react"; | ||
|
|
||
| const Box = () => { | ||
| const pi = Math.PI; | ||
| return ( | ||
| <mesh | ||
| position={[Math.sqrt(2), Math.sqrt(2), Math.sqrt(2)]} | ||
| rotateX={Math.PI / 2} | ||
| rotateY={pi / 2} | ||
| > | ||
| <boxGeometry args={[1, 1, 1]} /> | ||
| <meshStandardMaterial color="pink" /> | ||
| </mesh> | ||
| ); | ||
| }; | ||
|
|
||
| export default memo(Box); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -46,7 +46,7 @@ | |
|
|
||
| const { props } = getJsxElementPropTypes(sceneObject); | ||
|
|
||
| expect(props).toMatchInlineSnapshot(` | ||
|
Check failure on line 49 in packages/@triplex/server/src/ast/__tests__/type-infer.test.ts
|
||
| [ | ||
| { | ||
| "column": 9, | ||
|
|
@@ -1201,4 +1201,37 @@ | |
| ] | ||
| `); | ||
| }); | ||
| it("should evaluate expressions", () => { | ||
| const project = _createProject({ | ||
| tsConfigFilePath: join(__dirname, "__mocks__/tsconfig.json"), | ||
| }); | ||
| const sourceFile = project.addSourceFileAtPath( | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: you can use tbh I would have done that but didn't realize it existed until much later |
||
| join(__dirname, "__mocks__/prop-expression.tsx"), | ||
| ); | ||
| const sceneObject = getJsxElementAt(sourceFile, 12, 5); | ||
| if (!sceneObject) { | ||
| throw new Error("not found"); | ||
| } | ||
|
|
||
| const { props } = getJsxElementPropTypes(sceneObject); | ||
| const rotateXProp = props.find((prop) => prop.name === "rotateX"); | ||
| expect(rotateXProp).toEqual( | ||
| expect.objectContaining({ | ||
| value: "Math.PI / 2", | ||
| valueKind: "number", | ||
| }), | ||
| ); | ||
| const rotateYProp = props.find((prop) => prop.name === "rotateY"); | ||
| expect(rotateYProp).toEqual( | ||
| expect.objectContaining({ | ||
| valueKind: "unhandled", | ||
| }), | ||
| ); | ||
| const positionProp = props.find((prop) => prop.name === "position"); | ||
| expect(positionProp).toEqual( | ||
| expect.objectContaining({ | ||
| value: ["Math.sqrt(2)", "Math.sqrt(2)", "Math.sqrt(2)"], | ||
| }), | ||
| ); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,7 @@ | |
| * This repository utilizes multiple licenses across different directories. To | ||
| * see this files license find the nearest LICENSE file up the source tree. | ||
| */ | ||
| import { evaluateNumericalExpression } from "@triplex/lib/math"; | ||
| import { | ||
| type AttributeValue, | ||
| type DeclaredProp, | ||
|
|
@@ -464,6 +465,10 @@ export function resolveExpressionValue( | |
| return { kind: "number", value: Number(expression.getLiteralText()) }; | ||
| } | ||
|
|
||
| if (expression && evaluateNumericalExpression(expression.getText())) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah cool so if we can evaluate it then we allow it, nice |
||
| return { kind: "number", value: expression.getText() }; | ||
| } | ||
|
|
||
| if (Node.isPrefixUnaryExpression(expression)) { | ||
| const operand = expression.getOperand(); | ||
| if (Node.isNumericLiteral(operand)) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| /** | ||
| * Copyright (c) 2022—present Michael Dougall. All rights reserved. | ||
| * | ||
| * This repository utilizes multiple licenses across different directories. To | ||
| * see this files license find the nearest LICENSE file up the source tree. | ||
| */ | ||
| export function evaluateNumericalExpression(expression: string): number | null { | ||
| try { | ||
| const func = new Function(`return ${expression}`); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. any thoughts on sanitizing this input
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh yep that's definitely a good idea |
||
| const result = func(); | ||
|
|
||
| if ( | ||
| typeof result === "number" && | ||
| !Number.isNaN(result) && | ||
| Number.isFinite(result) | ||
| ) { | ||
| return result; | ||
| } | ||
| return null; | ||
| } catch { | ||
| return null; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,14 +6,24 @@ | |
| */ | ||
|
|
||
| import { compose, on, type RendererElementProps } from "@triplex/bridge/client"; | ||
| import { isRawCodeExpression } from "@triplex/lib"; | ||
| import { fg } from "@triplex/lib/fg"; | ||
| import { evaluateNumericalExpression } from "@triplex/lib/math"; | ||
| import { useCallback, useEffect, useRef, useState } from "react"; | ||
|
|
||
| function useForceRender() { | ||
| const [, setState] = useState(0); | ||
| return useCallback(() => setState((prev) => prev + 1), []); | ||
| } | ||
|
|
||
| function unwrapPropValue(value: unknown): unknown { | ||
| if (isRawCodeExpression(value)) { | ||
| const evaluated = evaluateNumericalExpression(value.__expr); | ||
| return evaluated !== null ? evaluated : value; | ||
| } | ||
| return value; | ||
| } | ||
|
|
||
| export function useTemporaryProps( | ||
| meta: RendererElementProps["__meta"], | ||
| props: Record<string, unknown>, | ||
|
|
@@ -40,16 +50,18 @@ export function useTemporaryProps( | |
| } | ||
| }), | ||
| on("request-set-element-prop", (data) => { | ||
| const propValue = unwrapPropValue(data.propValue); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah because we receive the prop as the expression literal not the string 🤔 and this is only for setting the temporary value before it's persisted? what if we updated this code path to always take the evaluated value and then for the actual prop setting it takes the serializable prop values / expressions? before if I remember correctly it just assumes everything is one in the same, now it can be different
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good idea, I'll look into it :) |
||
|
|
||
| if (data.astPath === meta.astPath && fg("selection_ast_path")) { | ||
| intermediateProps.current[data.propName] = data.propValue; | ||
| intermediateProps.current[data.propName] = propValue; | ||
| forceRender(); | ||
| } else if ( | ||
| "column" in data && | ||
| data.column === meta.column && | ||
| data.line === meta.line && | ||
| data.path === meta.path | ||
| ) { | ||
| intermediateProps.current[data.propName] = data.propValue; | ||
| intermediateProps.current[data.propName] = propValue; | ||
| forceRender(); | ||
| } | ||
| }), | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any thoughts on how we could have this auto infer? e.g. when typing into the input it would change to expression mode if an expression is found (or alternatively/more simply, maybe it is always in expression mode for number inputs?) and then we can enable / disable features if the value is an expression vs. number literal (the latter enables dragging to change the value)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohh that's smart, I like the idea of having the input always be in expression mode since a number is an expression albeit a simple one. I think I'll have a second go at it cause this all could be simpler than having a toggle when I think about it. thx!