From bb6fece69427ab096716636539ccaf9b48c7c303 Mon Sep 17 00:00:00 2001 From: Sam Nolan Date: Wed, 5 Oct 2022 11:57:32 +1100 Subject: [PATCH] Restrict interfaces with projects for components --- .../src/components/SquiggleChart.tsx | 100 ++++++++++-------- .../components/src/lib/hooks/useSquiggle.ts | 53 ++++------ packages/components/test/cleanup.test.tsx | 41 +------ 3 files changed, 78 insertions(+), 116 deletions(-) diff --git a/packages/components/src/components/SquiggleChart.tsx b/packages/components/src/components/SquiggleChart.tsx index c48696bb..d9ab3168 100644 --- a/packages/components/src/components/SquiggleChart.tsx +++ b/packages/components/src/components/SquiggleChart.tsx @@ -11,15 +11,13 @@ import { useSquiggle } from "../lib/hooks"; import { SquiggleViewer } from "./SquiggleViewer"; import { JsImports } from "../lib/jsImports"; -export interface SquiggleChartProps { +export type SquiggleChartProps = { /** The input string for squiggle */ - code?: string; + code: string; /** Allows to re-run the code if code hasn't changed */ executionId?: number; /** If the output requires monte carlo sampling, the amount of samples */ sampleCount?: number; - /** The amount of points returned to draw the distribution */ - environment?: environment; /** If the result is a function, where the function domain starts */ diagramStart?: number; /** If the result is a function, where the function domain ends */ @@ -54,50 +52,66 @@ export interface SquiggleChartProps { /** Whether to show vega actions to the user, so they can copy the chart spec */ distributionChartActions?: boolean; enableLocalSettings?: boolean; - /** The project that this execution is part of */ - project?: SqProject; - /** The name of the squiggle execution source. Generates a UUID if not given */ - sourceName?: string; - /** The sources that this execution continues */ - includes?: string[]; -} +} & (StandaloneExecutionProps | ProjectExecutionProps); +// Props needed for a standalone execution +type StandaloneExecutionProps = { + /** Project must be undefined */ + project?: undefined; + /** Includes must be undefined */ + includes?: undefined; + /** The amount of points returned to draw the distribution, not needed if using a project */ + environment?: environment; +}; + +// Props needed when executing inside a project. +type ProjectExecutionProps = { + /** environment must be undefined (we don't set it here, users can set the environment outside the execution) */ + environment?: undefined; + /** The project that this execution is part of */ + project: SqProject; + /** What other squiggle sources from the project to include. Default none */ + includes?: string[]; +}; const defaultOnChange = () => {}; const defaultImports: JsImports = {}; export const SquiggleChart: React.FC = React.memo( - ({ - code, - executionId = 0, - environment, - onChange = defaultOnChange, // defaultOnChange must be constant, don't move its definition here - height = 200, - jsImports = defaultImports, - showSummary = false, - width, - logX = false, - expY = false, - diagramStart = 0, - diagramStop = 10, - diagramCount = 20, - tickFormat, - minX, - maxX, - color, - title, - xAxisType = "number", - distributionChartActions, - enableLocalSettings = false, - sourceName, - includes = [], - project = SqProject.create(), - }) => { - const { result, bindings } = useSquiggle({ - sourceName, - includes, - project, + (props: SquiggleChartProps) => { + const { + executionId = 0, + onChange = defaultOnChange, // defaultOnChange must be constant, don't move its definition here + height = 200, + jsImports = defaultImports, + showSummary = false, + width, + logX = false, + expY = false, + diagramStart = 0, + diagramStop = 10, + diagramCount = 20, + tickFormat, + minX, + maxX, + color, + title, + xAxisType = "number", + distributionChartActions, + enableLocalSettings = false, + project = SqProject.create(), + code, + includes = [], + } = props; + + const p = project ?? SqProject.create(); + if (!project && props.environment) { + p.setEnvironment(props.environment); + } + + const { result, bindings } = useSquiggle({ + includes, + project: p, code, - environment, jsImports, onChange, executionId, @@ -133,7 +147,7 @@ export const SquiggleChart: React.FC = React.memo( height={height} distributionPlotSettings={distributionPlotSettings} chartSettings={chartSettings} - environment={environment ?? defaultEnvironment} + environment={p.getEnvironment()} enableLocalSettings={enableLocalSettings} /> ); diff --git a/packages/components/src/lib/hooks/useSquiggle.ts b/packages/components/src/lib/hooks/useSquiggle.ts index 826c0987..86126a8a 100644 --- a/packages/components/src/lib/hooks/useSquiggle.ts +++ b/packages/components/src/lib/hooks/useSquiggle.ts @@ -7,9 +7,7 @@ type SquiggleArgs = { code?: string; executionId?: number; jsImports?: JsImports; - environment?: environment; project: SqProject; - sourceName?: string; includes: string[]; onChange?: (expr: SqValue | undefined, sourceName: string) => void; }; @@ -17,32 +15,15 @@ type SquiggleArgs = { const importSourceName = (sourceName: string) => "imports-" + sourceName; export const useSquiggle = (args: SquiggleArgs) => { - const autogenName = useMemo(() => uuid.v4(), []); + const sourceName = useMemo(() => uuid.v4(), []); const result = useMemo( () => { const project = args.project; let needsClean = true; - let sourceName = ""; - // If the user specified a source and it already exists, assume we don't - // own the source - if (args.sourceName && project.getSource(args.sourceName)) { - needsClean = false; - sourceName = args.sourceName; - } else { - // Otherwise create a source, either with the name given or an automatic one - if (args.sourceName) { - sourceName = args.sourceName; - } else { - sourceName = autogenName; - } - project.setSource(sourceName, args.code ?? ""); - } + project.setSource(sourceName, args.code ?? ""); let includes = args.includes; - if (args.environment) { - project.setEnvironment(args.environment); - } if (args.jsImports && Object.keys(args.jsImports).length) { const importsSource = jsImportsToSquiggleCode(args.jsImports); project.setSource(importSourceName(sourceName), importsSource); @@ -54,8 +35,18 @@ export const useSquiggle = (args: SquiggleArgs) => { const bindings = project.getBindings(sourceName); return { result, bindings, sourceName, needsClean }; }, + // This complains about executionId not being used inside the function body. + // This is on purpose, as executionId simply allows you to run the squiggle + // code again // eslint-disable-next-line react-hooks/exhaustive-deps - [args.code, args.environment, args.jsImports, args.executionId] + [ + args.code, + args.jsImports, + args.executionId, + sourceName, + args.includes, + args.project, + ] ); const { onChange } = args; @@ -67,17 +58,13 @@ export const useSquiggle = (args: SquiggleArgs) => { ); }, [result, onChange]); - useEffect( - () => { - return () => { - if (result.needsClean) args.project.removeSource(result.sourceName); - if (args.project.getSource(importSourceName(result.sourceName))) - args.project.removeSource(result.sourceName); - }; - }, - // eslint-disable-next-line react-hooks/exhaustive-deps - [] - ); + useEffect(() => { + return () => { + args.project.removeSource(result.sourceName); + if (args.project.getSource(importSourceName(result.sourceName))) + args.project.removeSource(result.sourceName); + }; + }, [args.project, result.sourceName]); return result; }; diff --git a/packages/components/test/cleanup.test.tsx b/packages/components/test/cleanup.test.tsx index b9b0c29f..7bd109e4 100644 --- a/packages/components/test/cleanup.test.tsx +++ b/packages/components/test/cleanup.test.tsx @@ -10,6 +10,7 @@ test("Creates and cleans up source with no name", async () => { const { unmount } = render( ); + expect(project.getSourceIds().length).toBe(1); const sourceId = project.getSourceIds()[0]; @@ -19,43 +20,3 @@ test("Creates and cleans up source with no name", async () => { expect(project.getSourceIds().length).toBe(0); expect(project.getSource(sourceId)).toBe(undefined); }); - -test("Does not clean up existing source", async () => { - const project = SqProject.create(); - - project.setSource("main", "normal(0, 1)"); - - const { unmount } = render( - - ); - - expect(project.getSourceIds()).toStrictEqual(["main"]); - - const sourceId = project.getSourceIds()[0]; - expect(project.getSource(sourceId)).toBe("normal(0, 1)"); - - unmount(); - expect(project.getSourceIds()).toStrictEqual(["main"]); - expect(project.getSource(sourceId)).toBe("normal(0, 1)"); -}); - -test("Does clean up when given non-existant source", async () => { - const project = SqProject.create(); - - const { unmount } = render( - - ); - - expect(project.getSourceIds()).toStrictEqual(["main"]); - - const sourceId = project.getSourceIds()[0]; - expect(project.getSource(sourceId)).toBe("normal(0, 1)"); - - unmount(); - expect(project.getSourceIds()).toStrictEqual([]); - expect(project.getSource(sourceId)).toBe(undefined); -});