From 834372fd8c46c0e9bce695eb191384ac621fc88d Mon Sep 17 00:00:00 2001 From: Ozzie Gooen Date: Sun, 12 Nov 2023 10:07:13 -0800 Subject: [PATCH 01/12] Made Calculator inputs argument optional, for cases where zero params are needed --- packages/squiggle-lang/src/fr/calculator.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/squiggle-lang/src/fr/calculator.ts b/packages/squiggle-lang/src/fr/calculator.ts index 96d6ca5f7b..f0161a2d24 100644 --- a/packages/squiggle-lang/src/fr/calculator.ts +++ b/packages/squiggle-lang/src/fr/calculator.ts @@ -29,7 +29,7 @@ export const library = [ ["fn", frLambda], ["title", frOptional(frString)], ["description", frOptional(frString)], - ["inputs", frArray(frInput)], + ["inputs", frOptional(frArray(frInput))], ["autorun", frOptional(frBool)], ["sampleCount", frOptional(frNumber)] ), @@ -39,7 +39,7 @@ export const library = [ fn, title: title || undefined, description: description || undefined, - inputs: inputs, + inputs: inputs || [], autorun: autorun == null ? true : autorun, sampleCount: sampleCount || undefined, }); From 8e9ea0d2f3f50d326b569485b4aa37e6b64b4ab8 Mon Sep 17 00:00:00 2001 From: Ozzie Gooen Date: Sun, 12 Nov 2023 13:52:44 -0800 Subject: [PATCH 02/12] Calculator functions should not update when fields change --- .../Calculator/CalculatorResult.tsx | 26 +++++++++++++++++-- .../SquiggleChart/Calculator.stories.tsx | 10 ++++--- .../stories/SquigglePlayground.stories.tsx | 7 +++-- 3 files changed, 35 insertions(+), 8 deletions(-) diff --git a/packages/components/src/components/Calculator/CalculatorResult.tsx b/packages/components/src/components/Calculator/CalculatorResult.tsx index e8a870c199..b13b04adfb 100644 --- a/packages/components/src/components/Calculator/CalculatorResult.tsx +++ b/packages/components/src/components/Calculator/CalculatorResult.tsx @@ -1,4 +1,4 @@ -import { FC, useEffect, useMemo, useState, useCallback } from "react"; +import { FC, useEffect, useMemo, useState, useCallback, useRef } from "react"; import { Env, SqValue } from "@quri/squiggle-lang"; @@ -21,6 +21,18 @@ type Props = { autorun: boolean; }; +function useDeepCompareMemoize( + value: PlaygroundSettings +): PlaygroundSettings | undefined { + const ref = useRef(); + + if (JSON.stringify(value) !== JSON.stringify(ref.current)) { + ref.current = value; + } + + return ref.current; +} + export const CalculatorResult: FC = ({ valueWithContext, inputResults, @@ -29,6 +41,7 @@ export const CalculatorResult: FC = ({ processAllFieldCodes, autorun, }) => { + const _settings = useDeepCompareMemoize(settings); const [savedState, updateSavedState] = useSavedCalculatorState(valueWithContext); @@ -38,6 +51,15 @@ export const CalculatorResult: FC = ({ SqValueResult | undefined >(() => savedState?.calculatorResult); + const valueResultViewer = useMemo(() => { + return ( + !!calculatorResult && + _settings && ( + + ) + ); + }, [calculatorResult, _settings]); + const runCalculator = useCallback(() => { !autorun && processAllFieldCodes(); const parameters: SqValue[] = []; @@ -82,7 +104,7 @@ export const CalculatorResult: FC = ({ {calculatorResult ? (
Result
- + {valueResultViewer}
) : null} diff --git a/packages/components/src/stories/SquiggleChart/Calculator.stories.tsx b/packages/components/src/stories/SquiggleChart/Calculator.stories.tsx index b04647d00d..2b28e6a9e9 100644 --- a/packages/components/src/stories/SquiggleChart/Calculator.stories.tsx +++ b/packages/components/src/stories/SquiggleChart/Calculator.stories.tsx @@ -11,10 +11,9 @@ type Story = StoryObj; export const Basic: Story = { args: { code: ` - f(a, b, c, d) = [a,b,c,d] - - a = "A longer description of the calculator goes here...\n" - + f(a, b, c, d) = {|f| normal(f*10+b, 1)} + a = "A longer description of the calculator goes here... + " Calculator.make( { fn: f, @@ -26,8 +25,11 @@ export const Basic: Story = { Input.text({name: "Variable1", description: "This is a very long description This is a very long description This is a very long description This is a very long description This is a very long description", default: 1}), Input.select({name: "Variable3", default: "alice", options: ["alice", "charles", "bob", "bill", "maven", "billy", "samantha", "becky"]}) ], + sampleCount: 1000, + autorun: false } ) + `, }, }; diff --git a/packages/components/src/stories/SquigglePlayground.stories.tsx b/packages/components/src/stories/SquigglePlayground.stories.tsx index cbe7fb3bd6..a356ad3bff 100644 --- a/packages/components/src/stories/SquigglePlayground.stories.tsx +++ b/packages/components/src/stories/SquigglePlayground.stories.tsx @@ -234,7 +234,9 @@ bar = 123 export const Calculator: Story = { name: "Calculator", args: { - defaultCode: `f(a, b, c, d) = [a,b,c,d] + defaultCode: `calculatorCorrectlyUpdatesTest = 343 //changing this should propery update the calculator + + f(a, b, c, d) = {|f| normal(f*b, f*b)} a = "A longer description of the calculator goes here...\n" Calculator.make( { @@ -247,7 +249,8 @@ Calculator.make( Input.text({name: "Variable1", description: "This is a very long description This is a very long description This is a very long description This is a very long description This is a very long description", default: 1}), Input.select({name: "Variable3", default: "alice", options: ["alice", "charles", "bob", "bill", "maven", "billy", "samantha", "becky"]}) ], - sampleCount: 1000 + sampleCount: 1000, + autorun: false, } ) `, From 31576d88014fe5979fed9800e46bc71de3ea0397 Mon Sep 17 00:00:00 2001 From: Ozzie Gooen Date: Sun, 12 Nov 2023 13:59:28 -0800 Subject: [PATCH 03/12] Added extra calculator story --- .../SquiggleChart/Calculator.stories.tsx | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/packages/components/src/stories/SquiggleChart/Calculator.stories.tsx b/packages/components/src/stories/SquiggleChart/Calculator.stories.tsx index 2b28e6a9e9..8b2035de24 100644 --- a/packages/components/src/stories/SquiggleChart/Calculator.stories.tsx +++ b/packages/components/src/stories/SquiggleChart/Calculator.stories.tsx @@ -9,6 +9,32 @@ export default meta; type Story = StoryObj; export const Basic: Story = { + args: { + code: ` + f(a, b, c, d) = {|f| normal(f*10+b, 1)} + a = "A longer description of the calculator goes here... + " + Calculator.make( + { + fn: f, + title: "My Calculator", + description: a, + inputs: [ + Input.checkbox({name: "VariableCheckbox", description: "This is a long name", default: false}), + Input.textArea({name: "Variable2", description: "This is a long name", default: "2 to 40"}), + Input.text({name: "Variable1", description: "This is a very long description This is a very long description This is a very long description This is a very long description This is a very long description", default: 1}), + Input.select({name: "Variable3", default: "alice", options: ["alice", "charles", "bob", "bill", "maven", "billy", "samantha", "becky"]}) + ], + sampleCount: 1000, + autorun: true + } + ) + +`, + }, +}; + +export const WithoutAutorun: Story = { args: { code: ` f(a, b, c, d) = {|f| normal(f*10+b, 1)} From 12cece6d342a5b4e7bc49046485b5e261d386583 Mon Sep 17 00:00:00 2001 From: Ozzie Gooen Date: Mon, 13 Nov 2023 10:17:25 -0800 Subject: [PATCH 04/12] Updates based on PR comments --- .../Calculator/CalculatorResult.tsx | 21 ++------- .../src/components/Calculator/index.tsx | 46 +++++++++++++------ .../SquiggleChart/Calculator.stories.tsx | 2 +- .../stories/SquigglePlayground.stories.tsx | 4 +- 4 files changed, 39 insertions(+), 34 deletions(-) diff --git a/packages/components/src/components/Calculator/CalculatorResult.tsx b/packages/components/src/components/Calculator/CalculatorResult.tsx index b13b04adfb..e2aa3c11d2 100644 --- a/packages/components/src/components/Calculator/CalculatorResult.tsx +++ b/packages/components/src/components/Calculator/CalculatorResult.tsx @@ -1,4 +1,4 @@ -import { FC, useEffect, useMemo, useState, useCallback, useRef } from "react"; +import { FC, useEffect, useMemo, useState, useCallback } from "react"; import { Env, SqValue } from "@quri/squiggle-lang"; @@ -21,18 +21,6 @@ type Props = { autorun: boolean; }; -function useDeepCompareMemoize( - value: PlaygroundSettings -): PlaygroundSettings | undefined { - const ref = useRef(); - - if (JSON.stringify(value) !== JSON.stringify(ref.current)) { - ref.current = value; - } - - return ref.current; -} - export const CalculatorResult: FC = ({ valueWithContext, inputResults, @@ -41,7 +29,6 @@ export const CalculatorResult: FC = ({ processAllFieldCodes, autorun, }) => { - const _settings = useDeepCompareMemoize(settings); const [savedState, updateSavedState] = useSavedCalculatorState(valueWithContext); @@ -54,11 +41,11 @@ export const CalculatorResult: FC = ({ const valueResultViewer = useMemo(() => { return ( !!calculatorResult && - _settings && ( - + settings && ( + ) ); - }, [calculatorResult, _settings]); + }, [calculatorResult, settings]); const runCalculator = useCallback(() => { !autorun && processAllFieldCodes(); diff --git a/packages/components/src/components/Calculator/index.tsx b/packages/components/src/components/Calculator/index.tsx index ca96e3066f..792f47cf2d 100644 --- a/packages/components/src/components/Calculator/index.tsx +++ b/packages/components/src/components/Calculator/index.tsx @@ -1,4 +1,4 @@ -import { FC, useCallback, useEffect, useMemo, useState } from "react"; +import { FC, useCallback, useEffect, useMemo, useState, useRef } from "react"; import { FormProvider, useForm } from "react-hook-form"; import ReactMarkdown from "react-markdown"; @@ -199,6 +199,16 @@ export const CalculatorSampleCountValidation: React.FC<{ ); }; +function useDeepCompareMemoize(value: PlaygroundSettings): PlaygroundSettings { + const ref = useRef(); + + if (JSON.stringify(value) !== JSON.stringify(ref.current)) { + ref.current = value; + } + + return ref.current as PlaygroundSettings; +} + export const Calculator: FC = ({ environment, settings, @@ -209,22 +219,30 @@ export const Calculator: FC = ({ [environment, valueWithContext] ); + const _settings: PlaygroundSettings = useDeepCompareMemoize(settings); + const { calculator, form, inputResults, processAllFieldCodes } = useCalculator(valueWithContext, _environment); - const inputResultSettings: PlaygroundSettings = { - ...settings, - distributionChartSettings: { - ...settings.distributionChartSettings, - showSummary: false, - }, - chartHeight: 30, - }; - - const calculatorResultSettings: PlaygroundSettings = { - ...settings, - chartHeight: 200, - }; + const inputResultSettings: PlaygroundSettings = useMemo( + () => ({ + ..._settings, + distributionChartSettings: { + ..._settings.distributionChartSettings, + showSummary: false, + }, + chartHeight: 30, + }), + [_settings] + ); + + const calculatorResultSettings: PlaygroundSettings = useMemo( + () => ({ + ..._settings, + chartHeight: 200, + }), + [_settings] + ); const hasTitleOrDescription = !!calculator.title || !!calculator.description; diff --git a/packages/components/src/stories/SquiggleChart/Calculator.stories.tsx b/packages/components/src/stories/SquiggleChart/Calculator.stories.tsx index 8b2035de24..bf7592e766 100644 --- a/packages/components/src/stories/SquiggleChart/Calculator.stories.tsx +++ b/packages/components/src/stories/SquiggleChart/Calculator.stories.tsx @@ -11,7 +11,7 @@ type Story = StoryObj; export const Basic: Story = { args: { code: ` - f(a, b, c, d) = {|f| normal(f*10+b, 1)} + f(a, b, c, d) = [{|f| normal(f*10+b, 1)},a,b,c,d] a = "A longer description of the calculator goes here... " Calculator.make( diff --git a/packages/components/src/stories/SquigglePlayground.stories.tsx b/packages/components/src/stories/SquigglePlayground.stories.tsx index a356ad3bff..ee681598d8 100644 --- a/packages/components/src/stories/SquigglePlayground.stories.tsx +++ b/packages/components/src/stories/SquigglePlayground.stories.tsx @@ -227,7 +227,7 @@ bar = 123 \`\`test123\`\` " `, - height: 800, + height: 1200, }, }; @@ -236,7 +236,7 @@ export const Calculator: Story = { args: { defaultCode: `calculatorCorrectlyUpdatesTest = 343 //changing this should propery update the calculator - f(a, b, c, d) = {|f| normal(f*b, f*b)} + f(a, b, c, d) = [a,b,c,d,{|f| normal(f*b, f*b)}] a = "A longer description of the calculator goes here...\n" Calculator.make( { From 3246361c9473a94fde50fdd798bb000edd2c2458 Mon Sep 17 00:00:00 2001 From: Ozzie Gooen Date: Mon, 13 Nov 2023 15:04:59 -0800 Subject: [PATCH 05/12] Cleaned up memoization for ValueResultViewer --- .../Calculator/CalculatorResult.tsx | 26 +++++++------ .../src/components/Calculator/index.tsx | 38 ++++++------------- 2 files changed, 27 insertions(+), 37 deletions(-) diff --git a/packages/components/src/components/Calculator/CalculatorResult.tsx b/packages/components/src/components/Calculator/CalculatorResult.tsx index e2aa3c11d2..0817916ba9 100644 --- a/packages/components/src/components/Calculator/CalculatorResult.tsx +++ b/packages/components/src/components/Calculator/CalculatorResult.tsx @@ -1,4 +1,4 @@ -import { FC, useEffect, useMemo, useState, useCallback } from "react"; +import { FC, useEffect, useMemo, useState, useCallback, memo } from "react"; import { Env, SqValue } from "@quri/squiggle-lang"; @@ -21,6 +21,16 @@ type Props = { autorun: boolean; }; +const MemoizedValueResultViewer = memo(function MemoizedValueResultViewer({ + calculatorResult, + settings, +}: { + calculatorResult: SqValueResult; + settings: PlaygroundSettings; +}) { + return ; +}); + export const CalculatorResult: FC = ({ valueWithContext, inputResults, @@ -38,15 +48,6 @@ export const CalculatorResult: FC = ({ SqValueResult | undefined >(() => savedState?.calculatorResult); - const valueResultViewer = useMemo(() => { - return ( - !!calculatorResult && - settings && ( - - ) - ); - }, [calculatorResult, settings]); - const runCalculator = useCallback(() => { !autorun && processAllFieldCodes(); const parameters: SqValue[] = []; @@ -91,7 +92,10 @@ export const CalculatorResult: FC = ({ {calculatorResult ? (
Result
- {valueResultViewer} +
) : null} diff --git a/packages/components/src/components/Calculator/index.tsx b/packages/components/src/components/Calculator/index.tsx index 792f47cf2d..a049226f4c 100644 --- a/packages/components/src/components/Calculator/index.tsx +++ b/packages/components/src/components/Calculator/index.tsx @@ -1,4 +1,4 @@ -import { FC, useCallback, useEffect, useMemo, useState, useRef } from "react"; +import { FC, useCallback, useEffect, useMemo, useState } from "react"; import { FormProvider, useForm } from "react-hook-form"; import ReactMarkdown from "react-markdown"; @@ -199,16 +199,6 @@ export const CalculatorSampleCountValidation: React.FC<{ ); }; -function useDeepCompareMemoize(value: PlaygroundSettings): PlaygroundSettings { - const ref = useRef(); - - if (JSON.stringify(value) !== JSON.stringify(ref.current)) { - ref.current = value; - } - - return ref.current as PlaygroundSettings; -} - export const Calculator: FC = ({ environment, settings, @@ -219,29 +209,25 @@ export const Calculator: FC = ({ [environment, valueWithContext] ); - const _settings: PlaygroundSettings = useDeepCompareMemoize(settings); - const { calculator, form, inputResults, processAllFieldCodes } = useCalculator(valueWithContext, _environment); - const inputResultSettings: PlaygroundSettings = useMemo( - () => ({ - ..._settings, - distributionChartSettings: { - ..._settings.distributionChartSettings, - showSummary: false, - }, - chartHeight: 30, - }), - [_settings] - ); + const inputResultSettings: PlaygroundSettings = { + ...settings, + distributionChartSettings: { + ...settings.distributionChartSettings, + showSummary: false, + }, + chartHeight: 30, + }; + //This memoization is useful to make sure that CalculatorResult ResultViewer doesn't get updated too frequently. const calculatorResultSettings: PlaygroundSettings = useMemo( () => ({ - ..._settings, + ...settings, chartHeight: 200, }), - [_settings] + [settings] ); const hasTitleOrDescription = !!calculator.title || !!calculator.description; From 8ec70726e4e045673776ff5328a1cba0b8b17fe5 Mon Sep 17 00:00:00 2001 From: Ozzie Gooen Date: Mon, 13 Nov 2023 15:33:20 -0800 Subject: [PATCH 06/12] Add deep compare back in --- .../src/components/Calculator/index.tsx | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/packages/components/src/components/Calculator/index.tsx b/packages/components/src/components/Calculator/index.tsx index a049226f4c..74db98ed95 100644 --- a/packages/components/src/components/Calculator/index.tsx +++ b/packages/components/src/components/Calculator/index.tsx @@ -1,4 +1,4 @@ -import { FC, useCallback, useEffect, useMemo, useState } from "react"; +import { FC, useCallback, useEffect, useMemo, useRef, useState } from "react"; import { FormProvider, useForm } from "react-hook-form"; import ReactMarkdown from "react-markdown"; @@ -199,6 +199,16 @@ export const CalculatorSampleCountValidation: React.FC<{ ); }; +function useDeepCompareMemoize(value: PlaygroundSettings): PlaygroundSettings { + const ref = useRef(); + + if (JSON.stringify(value) !== JSON.stringify(ref.current)) { + ref.current = value; + } + + return ref.current as PlaygroundSettings; +} + export const Calculator: FC = ({ environment, settings, @@ -212,6 +222,8 @@ export const Calculator: FC = ({ const { calculator, form, inputResults, processAllFieldCodes } = useCalculator(valueWithContext, _environment); + const _settings: PlaygroundSettings = useDeepCompareMemoize(settings); + const inputResultSettings: PlaygroundSettings = { ...settings, distributionChartSettings: { @@ -224,10 +236,10 @@ export const Calculator: FC = ({ //This memoization is useful to make sure that CalculatorResult ResultViewer doesn't get updated too frequently. const calculatorResultSettings: PlaygroundSettings = useMemo( () => ({ - ...settings, + ..._settings, chartHeight: 200, }), - [settings] + [_settings] ); const hasTitleOrDescription = !!calculator.title || !!calculator.description; From 652064e91cefce4952d46acb1c7be15e27a6f160 Mon Sep 17 00:00:00 2001 From: Ozzie Gooen Date: Mon, 13 Nov 2023 15:37:31 -0800 Subject: [PATCH 07/12] Minor changes --- packages/components/src/components/Calculator/index.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/components/src/components/Calculator/index.tsx b/packages/components/src/components/Calculator/index.tsx index 74db98ed95..26ed8e6c5c 100644 --- a/packages/components/src/components/Calculator/index.tsx +++ b/packages/components/src/components/Calculator/index.tsx @@ -225,9 +225,9 @@ export const Calculator: FC = ({ const _settings: PlaygroundSettings = useDeepCompareMemoize(settings); const inputResultSettings: PlaygroundSettings = { - ...settings, + ..._settings, distributionChartSettings: { - ...settings.distributionChartSettings, + ..._settings.distributionChartSettings, showSummary: false, }, chartHeight: 30, From ac0435953ccf734e2d118c4bf907b8e2b6fb4671 Mon Sep 17 00:00:00 2001 From: Ozzie Gooen Date: Wed, 15 Nov 2023 06:16:49 -0800 Subject: [PATCH 08/12] Memoize ValueResultViewer directly --- .../components/Calculator/CalculatorResult.tsx | 17 ++--------------- .../components/Calculator/ValueResultViewer.tsx | 6 ++++-- 2 files changed, 6 insertions(+), 17 deletions(-) diff --git a/packages/components/src/components/Calculator/CalculatorResult.tsx b/packages/components/src/components/Calculator/CalculatorResult.tsx index 0817916ba9..e8a870c199 100644 --- a/packages/components/src/components/Calculator/CalculatorResult.tsx +++ b/packages/components/src/components/Calculator/CalculatorResult.tsx @@ -1,4 +1,4 @@ -import { FC, useEffect, useMemo, useState, useCallback, memo } from "react"; +import { FC, useEffect, useMemo, useState, useCallback } from "react"; import { Env, SqValue } from "@quri/squiggle-lang"; @@ -21,16 +21,6 @@ type Props = { autorun: boolean; }; -const MemoizedValueResultViewer = memo(function MemoizedValueResultViewer({ - calculatorResult, - settings, -}: { - calculatorResult: SqValueResult; - settings: PlaygroundSettings; -}) { - return ; -}); - export const CalculatorResult: FC = ({ valueWithContext, inputResults, @@ -92,10 +82,7 @@ export const CalculatorResult: FC = ({ {calculatorResult ? (
Result
- +
) : null} diff --git a/packages/components/src/components/Calculator/ValueResultViewer.tsx b/packages/components/src/components/Calculator/ValueResultViewer.tsx index 64aadcd6ef..cab8260e8f 100644 --- a/packages/components/src/components/Calculator/ValueResultViewer.tsx +++ b/packages/components/src/components/Calculator/ValueResultViewer.tsx @@ -1,4 +1,4 @@ -import { FC } from "react"; +import { FC, memo } from "react"; import { valueHasContext } from "../../lib/utility.js"; import { PlaygroundSettings } from "../PlaygroundSettings.js"; @@ -6,7 +6,7 @@ import { getSqValueWidget } from "../SquiggleViewer/getSqValueWidget.js"; import { SqValueResult } from "./types.js"; // Unlike ValueViewer/ValueWithContextViewer, this just renders the raw widget; TODO - better name? -export const ValueResultViewer: FC<{ +const ValueResultViewerBase: FC<{ result: SqValueResult; settings: PlaygroundSettings; }> = ({ result, settings }) => { @@ -25,3 +25,5 @@ export const ValueResultViewer: FC<{ ); } }; + +export const ValueResultViewer = memo(ValueResultViewerBase); From b3461ab2267f50d928065f954567063544360493 Mon Sep 17 00:00:00 2001 From: Vyacheslav Matyukhin Date: Wed, 15 Nov 2023 14:06:21 -0600 Subject: [PATCH 09/12] start ValueWithContextViewer refactoring, clean up calculators --- .../Calculator/CalculatorResult.tsx | 4 +- .../Calculator/ValueResultViewer.tsx | 13 +- .../src/components/Calculator/index.tsx | 20 +- .../SquiggleViewer/ValueWithContextViewer.tsx | 184 ++++++++++-------- .../src/components/SquiggleViewer/utils.ts | 2 +- 5 files changed, 119 insertions(+), 104 deletions(-) diff --git a/packages/components/src/components/Calculator/CalculatorResult.tsx b/packages/components/src/components/Calculator/CalculatorResult.tsx index e8a870c199..c5cdf4ab80 100644 --- a/packages/components/src/components/Calculator/CalculatorResult.tsx +++ b/packages/components/src/components/Calculator/CalculatorResult.tsx @@ -1,7 +1,8 @@ -import { FC, useEffect, useMemo, useState, useCallback } from "react"; +import { FC, useCallback, useEffect, useMemo, useState } from "react"; import { Env, SqValue } from "@quri/squiggle-lang"; +import { Button } from "@quri/ui"; import { PlaygroundSettings } from "../PlaygroundSettings.js"; import { ValueResultViewer } from "./ValueResultViewer.js"; import { @@ -10,7 +11,6 @@ import { SqValueResult, } from "./types.js"; import { useSavedCalculatorState } from "./useSavedCalculatorState.js"; -import { Button } from "@quri/ui"; type Props = { valueWithContext: SqCalculatorValueWithContext; diff --git a/packages/components/src/components/Calculator/ValueResultViewer.tsx b/packages/components/src/components/Calculator/ValueResultViewer.tsx index cab8260e8f..8cb02ddf82 100644 --- a/packages/components/src/components/Calculator/ValueResultViewer.tsx +++ b/packages/components/src/components/Calculator/ValueResultViewer.tsx @@ -1,4 +1,4 @@ -import { FC, memo } from "react"; +import { memo } from "react"; import { valueHasContext } from "../../lib/utility.js"; import { PlaygroundSettings } from "../PlaygroundSettings.js"; @@ -6,10 +6,13 @@ import { getSqValueWidget } from "../SquiggleViewer/getSqValueWidget.js"; import { SqValueResult } from "./types.js"; // Unlike ValueViewer/ValueWithContextViewer, this just renders the raw widget; TODO - better name? -const ValueResultViewerBase: FC<{ +export const ValueResultViewer = memo(function ValueResultViewer({ + result, + settings, +}: { result: SqValueResult; settings: PlaygroundSettings; -}> = ({ result, settings }) => { +}) { if (result.ok) { const value = result.value; if (valueHasContext(value)) { @@ -24,6 +27,4 @@ const ValueResultViewerBase: FC<{ ); } -}; - -export const ValueResultViewer = memo(ValueResultViewerBase); +}); diff --git a/packages/components/src/components/Calculator/index.tsx b/packages/components/src/components/Calculator/index.tsx index 26ed8e6c5c..1dd6d144bb 100644 --- a/packages/components/src/components/Calculator/index.tsx +++ b/packages/components/src/components/Calculator/index.tsx @@ -199,16 +199,6 @@ export const CalculatorSampleCountValidation: React.FC<{ ); }; -function useDeepCompareMemoize(value: PlaygroundSettings): PlaygroundSettings { - const ref = useRef(); - - if (JSON.stringify(value) !== JSON.stringify(ref.current)) { - ref.current = value; - } - - return ref.current as PlaygroundSettings; -} - export const Calculator: FC = ({ environment, settings, @@ -222,12 +212,10 @@ export const Calculator: FC = ({ const { calculator, form, inputResults, processAllFieldCodes } = useCalculator(valueWithContext, _environment); - const _settings: PlaygroundSettings = useDeepCompareMemoize(settings); - const inputResultSettings: PlaygroundSettings = { - ..._settings, + ...settings, distributionChartSettings: { - ..._settings.distributionChartSettings, + ...settings.distributionChartSettings, showSummary: false, }, chartHeight: 30, @@ -236,10 +224,10 @@ export const Calculator: FC = ({ //This memoization is useful to make sure that CalculatorResult ResultViewer doesn't get updated too frequently. const calculatorResultSettings: PlaygroundSettings = useMemo( () => ({ - ..._settings, + ...settings, chartHeight: 200, }), - [_settings] + [settings] ); const hasTitleOrDescription = !!calculator.title || !!calculator.description; diff --git a/packages/components/src/components/SquiggleViewer/ValueWithContextViewer.tsx b/packages/components/src/components/SquiggleViewer/ValueWithContextViewer.tsx index 1771d2f78c..c22b2e0cd7 100644 --- a/packages/components/src/components/SquiggleViewer/ValueWithContextViewer.tsx +++ b/packages/components/src/components/SquiggleViewer/ValueWithContextViewer.tsx @@ -1,5 +1,5 @@ import { clsx } from "clsx"; -import { FC, ReactNode, useState } from "react"; +import { FC, PropsWithChildren, ReactNode, useState } from "react"; import ReactMarkdown from "react-markdown"; import { SqValuePath } from "@quri/squiggle-lang"; @@ -33,6 +33,67 @@ export type SettingsMenuParams = { onChange: () => void; }; +function useComment(value: SqValueWithContext): string | undefined { + return value.context.docstring(); +} + +const CommentIconForValue: FC<{ value: SqValueWithContext }> = ({ value }) => { + const comment = useComment(value); + + return comment ? ( +
+ + + + + +
+ ) : null; +}; + +const WithComment: FC> = ({ + value, + children, +}) => { + const comment = useComment(value); + + if (!comment) { + return children; + } + + const tagsWithTopPosition = new Set([ + "Dict", + "Array", + "TableChart", + "Plot", + "String", + ]); + const commentPosition = tagsWithTopPosition.has(value.tag) ? "top" : "bottom"; + + const commentEl = ( + + {comment} + + ); + + return ( + // TODO - can be simplified with flex-col-reverse +
+ {commentPosition === "top" && commentEl} + {children} + {commentPosition === "bottom" && commentEl} +
+ ); +}; + type Props = { value: SqValueWithContext; }; @@ -78,13 +139,13 @@ export const ValueWithContextViewer: FC = ({ value }) => { const shouldCollapseChildren = childrenCount > 10; - function shouldCollapseElement() { + const shouldCollapseElement = () => { if (isRoot) { return childrenCount > 30; } else { return childrenCount > 5 || tagsDefaultCollapsed.has(tag); } - } + }; if (shouldCollapseChildren) { collapseChildren(value); @@ -170,7 +231,7 @@ export const ValueWithContextViewer: FC = ({ value }) => { findInEditor()} /> @@ -185,88 +246,53 @@ export const ValueWithContextViewer: FC = ({ value }) => { const headerSettingsButton = () => widget.renderSettingsMenu?.({ onChange: forceUpdate }); - const leftCollapseBorder = () => ( -
-
-
- ); - - const comment = value.context.docstring(); - const hasComment = comment && comment !== ""; - - const commentIcon = () => - comment && ( -
- - - - - -
- ); - - const tagsWithTopPosition = new Set([ - "Dict", - "Array", - "TableChart", - "Plot", - "String", - ]); - const commentPosition = tagsWithTopPosition.has(tag) ? "top" : "bottom"; - - const isDictOrList = tag === "Dict" || tag === "Array"; - - const showComment = () => - comment && ( - - {comment} - - ); + const leftCollapseBorder = () => { + const isDictOrList = tag === "Dict" || tag === "Array"; + if (isDictOrList) { + return ( +
+
+
+ ); + } else if (!isRoot) { + // non-root leaf elements have unclickable padding to align with dict/list elements + return
; // min-w-1rem = w-4 + } else { + return null; + } + }; return (
- {(name !== undefined || isRoot) && ( -
-
- {!isFocused && triangleToggle()} - {headerName} - {!isFocused && headerPreview()} - {!isFocused && !isOpen && commentIcon()} - {!isRoot && editor && headerFindInEditorButton()} -
-
- {isOpen && headerString()} - {isOpen && headerSettingsButton()} -
-
- )} +
+
+ {!isFocused && triangleToggle()} + {headerName} + {!isFocused && headerPreview()} + {!isFocused && !isOpen && } + {!isRoot && editor && headerFindInEditorButton()} +
+
+ {isOpen && headerString()} + {isOpen && headerSettingsButton()} +
+
{isOpen && (
- {!isFocused && isDictOrList && leftCollapseBorder()} - {!isFocused && !isDictOrList && !isRoot && ( -
// min-w-1rem = w-4 - )} + {!isFocused && leftCollapseBorder()}
- {commentPosition === "top" && hasComment && showComment()} - {render(getAdjustedMergedSettings(path))} - {commentPosition === "bottom" && hasComment && showComment()} + + {render(getAdjustedMergedSettings(path))} +
)} diff --git a/packages/components/src/components/SquiggleViewer/utils.ts b/packages/components/src/components/SquiggleViewer/utils.ts index 5dbed7155b..f6957214a0 100644 --- a/packages/components/src/components/SquiggleViewer/utils.ts +++ b/packages/components/src/components/SquiggleViewer/utils.ts @@ -51,7 +51,7 @@ export function pathAsString(path: SqValuePath) { } } -export function pathToShortName(path: SqValuePath): string | undefined { +export function pathToShortName(path: SqValuePath): string { if (isTopLevel(path)) { return topLevelName(path); } else { From d2660d74e7f9a7ae0aa142c3be034881af83e2f6 Mon Sep 17 00:00:00 2001 From: Vyacheslav Matyukhin Date: Wed, 15 Nov 2023 14:50:29 -0600 Subject: [PATCH 10/12] more ValueWithContextViewer cleanups; useMergedSettings hook with stable settings identity --- .../src/components/Calculator/index.tsx | 19 +-- .../SquiggleViewer/ItemSettingsMenu.tsx | 5 +- .../SquiggleViewer/ValueWithContextViewer.tsx | 121 +++++++++--------- .../SquiggleViewer/ViewerProvider.tsx | 49 +++---- .../SquiggleViewer/getSqValueWidget.tsx | 25 +++- .../src/components/SquiggleViewer/utils.ts | 7 +- 6 files changed, 113 insertions(+), 113 deletions(-) diff --git a/packages/components/src/components/Calculator/index.tsx b/packages/components/src/components/Calculator/index.tsx index 1dd6d144bb..f7fa011787 100644 --- a/packages/components/src/components/Calculator/index.tsx +++ b/packages/components/src/components/Calculator/index.tsx @@ -212,14 +212,17 @@ export const Calculator: FC = ({ const { calculator, form, inputResults, processAllFieldCodes } = useCalculator(valueWithContext, _environment); - const inputResultSettings: PlaygroundSettings = { - ...settings, - distributionChartSettings: { - ...settings.distributionChartSettings, - showSummary: false, - }, - chartHeight: 30, - }; + const inputResultSettings: PlaygroundSettings = useMemo( + () => ({ + ...settings, + distributionChartSettings: { + ...settings.distributionChartSettings, + showSummary: false, + }, + chartHeight: 30, + }), + [settings] + ); //This memoization is useful to make sure that CalculatorResult ResultViewer doesn't get updated too frequently. const calculatorResultSettings: PlaygroundSettings = useMemo( diff --git a/packages/components/src/components/SquiggleViewer/ItemSettingsMenu.tsx b/packages/components/src/components/SquiggleViewer/ItemSettingsMenu.tsx index 50042cb0e9..d5464abfc6 100644 --- a/packages/components/src/components/SquiggleViewer/ItemSettingsMenu.tsx +++ b/packages/components/src/components/SquiggleViewer/ItemSettingsMenu.tsx @@ -17,6 +17,7 @@ import { useSetLocalItemState, useViewerContext, useResetStateSettings, + useMergedSettings, } from "./ViewerProvider.js"; import { pathAsString } from "./utils.js"; @@ -38,11 +39,11 @@ const ItemSettingsModal: React.FC< resetScroll, }) => { const setLocalItemState = useSetLocalItemState(); - const { getLocalItemState, getMergedSettings } = useViewerContext(); + const { getLocalItemState } = useViewerContext(); const { path } = value.context; - const mergedSettings = getMergedSettings({ path }); + const mergedSettings = useMergedSettings(path); const form = useForm({ resolver: zodResolver(viewSettingsSchema), diff --git a/packages/components/src/components/SquiggleViewer/ValueWithContextViewer.tsx b/packages/components/src/components/SquiggleViewer/ValueWithContextViewer.tsx index c22b2e0cd7..3ebd4502a0 100644 --- a/packages/components/src/components/SquiggleViewer/ValueWithContextViewer.tsx +++ b/packages/components/src/components/SquiggleViewer/ValueWithContextViewer.tsx @@ -1,11 +1,10 @@ import { clsx } from "clsx"; -import { FC, PropsWithChildren, ReactNode, useState } from "react"; +import { FC, PropsWithChildren, useMemo, useState } from "react"; import ReactMarkdown from "react-markdown"; -import { SqValuePath } from "@quri/squiggle-lang"; import { - CommentIcon, CodeBracketIcon, + CommentIcon, TextTooltip, TriangleIcon, } from "@quri/ui"; @@ -17,28 +16,25 @@ import { useCollapseChildren, useFocus, useIsFocused, + useMergedSettings, useSetCollapsed, useToggleCollapsed, useViewerContext, } from "./ViewerProvider.js"; import { getSqValueWidget } from "./getSqValueWidget.js"; -import { - MergedItemSettings, - getChildrenValues, - pathToShortName, -} from "./utils.js"; +import { getChildrenValues, pathToShortName } from "./utils.js"; export type SettingsMenuParams = { // Used to notify this component that settings have changed, so that it could re-render itself. onChange: () => void; }; -function useComment(value: SqValueWithContext): string | undefined { +function getComment(value: SqValueWithContext): string | undefined { return value.context.docstring(); } const CommentIconForValue: FC<{ value: SqValueWithContext }> = ({ value }) => { - const comment = useComment(value); + const comment = getComment(value); return comment ? (
@@ -54,11 +50,12 @@ const CommentIconForValue: FC<{ value: SqValueWithContext }> = ({ value }) => { ) : null; }; -const WithComment: FC> = ({ - value, - children, -}) => { - const comment = useComment(value); +type Props = { + value: SqValueWithContext; +}; + +const WithComment: FC> = ({ value, children }) => { + const comment = getComment(value); if (!comment) { return children; @@ -94,8 +91,27 @@ const WithComment: FC> = ({ ); }; -type Props = { - value: SqValueWithContext; +const ValueViewerBody: FC = ({ value }) => { + const widget = getSqValueWidget(value); + + const { path } = value.context; + const isFocused = useIsFocused(path); + const isRoot = path.isRoot(); + + const mergedSettings = useMergedSettings(path); + const adjustedMergedSettings = useMemo(() => { + const { chartHeight } = mergedSettings; + return { + ...mergedSettings, + chartHeight: isFocused || isRoot ? chartHeight * 4 : chartHeight, + }; + }, [isFocused, isRoot, mergedSettings]); + + return ( + + {widget.render(adjustedMergedSettings)} + + ); }; export const ValueWithContextViewer: FC = ({ value }) => { @@ -103,28 +119,14 @@ export const ValueWithContextViewer: FC = ({ value }) => { const { path } = value.context; const widget = getSqValueWidget(value); - const heading = widget.heading || value.publicName(); - const hasChildren = () => !!getChildrenValues(value); - const render: (settings: MergedItemSettings) => ReactNode = - (value.tag === "Dict" || value.tag === "Array") && hasChildren() - ? (settings) => ( -
{widget.render(settings)}
- ) - : widget.render; const toggleCollapsed_ = useToggleCollapsed(); const setCollapsed = useSetCollapsed(); const collapseChildren = useCollapseChildren(); const focus = useFocus(); - const { editor, getLocalItemState, getMergedSettings, dispatch } = - useViewerContext(); + const { editor, getLocalItemState, dispatch } = useViewerContext(); const isFocused = useIsFocused(path); - const findInEditor = () => { - const location = value.context.findLocation(); - editor?.scrollTo(location.start.offset); - }; - // Since `ViewerContext` doesn't store settings, this component won't rerender when `setSettings` is called. // So we use `forceUpdate` to force rerendering. const forceUpdate = useForceUpdate(); @@ -132,6 +134,7 @@ export const ValueWithContextViewer: FC = ({ value }) => { const isRoot = path.isRoot(); // Collapse children and element if desired. Uses crude heuristics. + // TODO - this code has side effects, it'd be better if we ran it somewhere else, e.g. traverse values recursively when `ViewerProvider` is initialized. useState(() => { const tagsDefaultCollapsed = new Set(["Bool", "Number", "Void", "Input"]); // TODO - value.size() could be faster. @@ -155,24 +158,11 @@ export const ValueWithContextViewer: FC = ({ value }) => { } }); - const settings = getLocalItemState({ path }); - - const getAdjustedMergedSettings = (path: SqValuePath) => { - const mergedSettings = getMergedSettings({ path }); - const { chartHeight } = mergedSettings; - return { - ...mergedSettings, - chartHeight: isFocused || isRoot ? chartHeight * 4 : chartHeight, - }; - }; - const toggleCollapsed = () => { toggleCollapsed_(path); forceUpdate(); }; - const name = pathToShortName(path); - // We should switch to ref cleanups after https://github.com/facebook/react/pull/25686 is released. const saveRef = useEffectRef((element: HTMLDivElement) => { dispatch({ @@ -188,7 +178,7 @@ export const ValueWithContextViewer: FC = ({ value }) => { }; }); - const isOpen = isFocused || !settings.collapsed; + const isOpen = isFocused || !getLocalItemState({ path }).collapsed; const _focus = () => !isFocused && !isRoot && focus(path); const triangleToggle = () => ( @@ -210,6 +200,7 @@ export const ValueWithContextViewer: FC = ({ value }) => { } }; + const name = pathToShortName(path); const headerName = (
{name} @@ -226,23 +217,33 @@ export const ValueWithContextViewer: FC = ({ value }) => { {widget.renderPreview()}
); - const headerFindInEditorButton = () => ( -
- - - findInEditor()} - /> - - -
- ); + const headerFindInEditorButton = () => { + const findInEditor = () => { + const location = value.context.findLocation(); + editor?.scrollTo(location.start.offset); + }; + + return ( +
+ + + + + +
+ ); + }; + + const heading = widget.heading || value.publicName(); const headerString = () => (
{heading}
); + const headerSettingsButton = () => widget.renderSettingsMenu?.({ onChange: forceUpdate }); @@ -290,9 +291,7 @@ export const ValueWithContextViewer: FC = ({ value }) => {
{!isFocused && leftCollapseBorder()}
- - {render(getAdjustedMergedSettings(path))} - +
)} diff --git a/packages/components/src/components/SquiggleViewer/ViewerProvider.tsx b/packages/components/src/components/SquiggleViewer/ViewerProvider.tsx index d0ea20f3b1..49d3101f19 100644 --- a/packages/components/src/components/SquiggleViewer/ViewerProvider.tsx +++ b/packages/components/src/components/SquiggleViewer/ViewerProvider.tsx @@ -14,11 +14,11 @@ import { SqValue, SqValuePath } from "@quri/squiggle-lang"; import { PartialPlaygroundSettings, + PlaygroundSettings, defaultPlaygroundSettings, } from "../PlaygroundSettings.js"; import { LocalItemState, - MergedItemSettings, getChildrenValues, pathAsString, topLevelBindingsName, @@ -89,6 +89,7 @@ type ViewerContextShape = { // Note that we don't store localItemState themselves in the context (that would cause rerenders of the entire tree on each settings update). // Instead, we keep localItemState in local state and notify the global context via setLocalItemState to pass them down the component tree again if it got rebuilt from scratch. // See ./SquiggleViewer.tsx and ./ValueWithContextViewer.tsx for other implementation details on this. + globalSettings: PlaygroundSettings; getLocalItemState({ path, defaults, @@ -97,13 +98,6 @@ type ViewerContextShape = { defaults?: LocalItemState; }): LocalItemState; getCalculator({ path }: { path: SqValuePath }): CalculatorState | undefined; - getMergedSettings({ - path, - defaults, - }: { - path: SqValuePath; - defaults?: LocalItemState; - }): MergedItemSettings; localSettingsEnabled: boolean; // show local settings icon in the UI focused?: SqValuePath; editor?: CodeEditorHandle; @@ -111,9 +105,9 @@ type ViewerContextShape = { }; export const ViewerContext = createContext({ + globalSettings: defaultPlaygroundSettings, getLocalItemState: () => ({ collapsed: false, settings: {} }), getCalculator: () => undefined, - getMergedSettings: () => defaultPlaygroundSettings, localSettingsEnabled: false, focused: undefined, editor: undefined, @@ -199,15 +193,27 @@ export function useCollapseChildren() { ); } -export function useIsFocused(location: SqValuePath) { +export function useIsFocused(path: SqValuePath) { const { focused } = useViewerContext(); if (!focused) { return false; } else { - return pathAsString(focused) === pathAsString(location); + return pathAsString(focused) === pathAsString(path); } } +export function useMergedSettings(path: SqValuePath) { + const { getLocalItemState, globalSettings } = useViewerContext(); + + const localItemState = getLocalItemState({ path }); + + const result: PlaygroundSettings = useMemo( + () => merge({}, globalSettings, localItemState.settings), + [globalSettings, localItemState.settings] + ); + return result; +} + type LocalItemStateStore = { [k: string]: LocalItemState; }; @@ -284,25 +290,6 @@ export const ViewerProvider: FC< [localItemStateStoreRef] ); - const getMergedSettings = useCallback( - ({ - path, - defaults = defaultLocalItemState, - }: { - path: SqValuePath; - defaults?: LocalItemState; - }) => { - const localItemState = getLocalItemState({ path, defaults }); - const result: MergedItemSettings = merge( - {}, - globalSettings, - localItemState.settings - ); - return result; - }, - [globalSettings, getLocalItemState] - ); - const setCollapsed = (path: SqValuePath, isCollapsed: boolean) => { setLocalItemState(path, (state) => ({ ...state, @@ -378,9 +365,9 @@ export const ViewerProvider: FC< return ( ReactNode; renderSettingsMenu?: (params: SettingsMenuParams) => ReactNode; - render: (settings: MergedItemSettings) => ReactNode; + render: (settings: PlaygroundSettings) => ReactNode; }; export function getSqValueWidget(value: SqValueWithContext): ValueWidget { @@ -294,7 +297,13 @@ export function getSqValueWidget(value: SqValueWithContext): ValueWidget { renderPreview: () => ( ), - render: () => entries.map((r, i) => ), + render: () => ( +
+ {entries.map((r, i) => ( + + ))} +
+ ), }; } case "Array": { @@ -303,7 +312,13 @@ export function getSqValueWidget(value: SqValueWithContext): ValueWidget { return { heading: `List(${length})`, renderPreview: () => , - render: () => entries.map((r, i) => ), + render: () => ( +
+ {entries.map((r, i) => ( + + ))} +
+ ), }; } diff --git a/packages/components/src/components/SquiggleViewer/utils.ts b/packages/components/src/components/SquiggleViewer/utils.ts index f6957214a0..daeef61748 100644 --- a/packages/components/src/components/SquiggleViewer/utils.ts +++ b/packages/components/src/components/SquiggleViewer/utils.ts @@ -1,9 +1,6 @@ import { PathItem, SqValue, SqValuePath } from "@quri/squiggle-lang"; -import { - PartialPlaygroundSettings, - PlaygroundSettings, -} from "../PlaygroundSettings.js"; import { CalculatorState } from "../Calculator/types.js"; +import { PartialPlaygroundSettings } from "../PlaygroundSettings.js"; export type LocalItemState = { collapsed: boolean; @@ -14,8 +11,6 @@ export type LocalItemState = { >; }; -export type MergedItemSettings = PlaygroundSettings; - export const pathItemFormat = (item: PathItem): string => { if (item.type === "cellAddress") { return `Cell (${item.value.row},${item.value.column})`; From 92374ae2a03510ec8e4f4485d2251de73b118417 Mon Sep 17 00:00:00 2001 From: Ozzie Gooen Date: Wed, 15 Nov 2023 21:36:59 -0800 Subject: [PATCH 11/12] Remove useMemo for linter --- packages/components/src/components/Calculator/index.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/components/src/components/Calculator/index.tsx b/packages/components/src/components/Calculator/index.tsx index f7fa011787..488b65eba6 100644 --- a/packages/components/src/components/Calculator/index.tsx +++ b/packages/components/src/components/Calculator/index.tsx @@ -1,4 +1,4 @@ -import { FC, useCallback, useEffect, useMemo, useRef, useState } from "react"; +import { FC, useCallback, useEffect, useMemo, useState } from "react"; import { FormProvider, useForm } from "react-hook-form"; import ReactMarkdown from "react-markdown"; From f4166b2bdb139f0588cbd773316087a6eac9686f Mon Sep 17 00:00:00 2001 From: Ozzie Gooen Date: Wed, 15 Nov 2023 21:55:18 -0800 Subject: [PATCH 12/12] Added changeset --- .changeset/tidy-scissors-promise.md | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 .changeset/tidy-scissors-promise.md diff --git a/.changeset/tidy-scissors-promise.md b/.changeset/tidy-scissors-promise.md new file mode 100644 index 0000000000..823ab9deae --- /dev/null +++ b/.changeset/tidy-scissors-promise.md @@ -0,0 +1,6 @@ +--- +"@quri/squiggle-lang": patch +"@quri/squiggle-components": patch +--- + +Calculator result functions no longer re-run on input changes. Calculator inputs can be empty, if the function doesn't take any arguments