From e1aaacb4ee908023b4a563475272efca8fb94cfe Mon Sep 17 00:00:00 2001 From: Andrew Edstrom Date: Mon, 20 Nov 2023 21:34:44 -0700 Subject: [PATCH] chore(widget): make controller more functional (#182) * promote detail-navigation-header out of assets directory * processStep and processThing no longer side effect into the nodesById and edges they are passed * replace forEach with for ... of * improve test descriptions --- packages/widget/src/assets/index.ts | 1 - .../{assets => }/detail-navigation-header.ts | 2 +- packages/widget/src/docmap-controller.ts | 99 ++++++++++++------- packages/widget/src/docmaps-widget.ts | 3 +- .../test/unit/docmap-controller.test.ts | 18 ++-- 5 files changed, 76 insertions(+), 47 deletions(-) rename packages/widget/src/{assets => }/detail-navigation-header.ts (98%) diff --git a/packages/widget/src/assets/index.ts b/packages/widget/src/assets/index.ts index 4a272755..7ba1cd18 100644 --- a/packages/widget/src/assets/index.ts +++ b/packages/widget/src/assets/index.ts @@ -1,4 +1,3 @@ // All files which will be accessible when the widget is installed via npm are declared here export * from './close-details-button'; export * from './logo'; -export * from './detail-navigation-header'; \ No newline at end of file diff --git a/packages/widget/src/assets/detail-navigation-header.ts b/packages/widget/src/detail-navigation-header.ts similarity index 98% rename from packages/widget/src/assets/detail-navigation-header.ts rename to packages/widget/src/detail-navigation-header.ts index b348a3f2..8a5a1df4 100644 --- a/packages/widget/src/assets/detail-navigation-header.ts +++ b/packages/widget/src/detail-navigation-header.ts @@ -1,5 +1,5 @@ import { nothing, svg, SVGTemplateResult } from 'lit'; -import { DisplayObject, TYPE_DISPLAY_OPTIONS } from '../util'; +import { DisplayObject, TYPE_DISPLAY_OPTIONS } from './util'; const backButton = ( allNodes: DisplayObject[], diff --git a/packages/widget/src/docmap-controller.ts b/packages/widget/src/docmap-controller.ts index 923cfa74..14214e2b 100644 --- a/packages/widget/src/docmap-controller.ts +++ b/packages/widget/src/docmap-controller.ts @@ -59,18 +59,19 @@ function getOrderedSteps(docmap: DocmapT): StepT[] { return orderedSteps; } -export function stepsToGraph(steps: StepT[]): DisplayObjectGraph { - const nodesById: { [id: string]: DisplayObject } = {}; - const edges: DisplayObjectEdge[] = []; +interface NodesById { + [id: string]: DisplayObject; +} - let idCounter: number = 1; - const idGenerator = (): string => { - const newId = `n${idCounter}`; - idCounter++; - return newId; - }; +export function stepsToGraph(steps: StepT[]): DisplayObjectGraph { + let nodesById: NodesById = {}; + let edges: DisplayObjectEdge[] = []; - steps.forEach((step) => processStep(step, nodesById, edges, idGenerator)); + for (const step of steps) { + const { newNodesById, newEdges } = processStep(step, nodesById); + nodesById = newNodesById; + edges = [...edges, ...newEdges]; + } const nodes: DisplayObject[] = Object.values(nodesById); return { nodes, edges }; @@ -78,39 +79,62 @@ export function stepsToGraph(steps: StepT[]): DisplayObjectGraph { function processStep( step: StepT, - nodesById: { [id: string]: DisplayObject }, - edges: DisplayObjectEdge[], - generateId: () => string, -) { - const inputIds: string[] = - step.inputs?.map((input) => processThing(input, nodesById, [], generateId)) || []; - - step.actions.forEach((action) => { - action.outputs.forEach((output) => { - const outputId = processThing(output, nodesById, action.participants, generateId); - - inputIds.forEach((inputId: string) => { - edges.push({ sourceId: inputId, targetId: outputId }); - }); - }); - }); + nodesById: NodesById, +): { newNodesById: NodesById; newEdges: DisplayObjectEdge[] } { + let newNodesById: NodesById = { ...nodesById }; + let newEdges: DisplayObjectEdge[] = []; + + const inputIds = + step.inputs?.map((input) => { + const result = processThing(input, newNodesById); + newNodesById = result.newNodesById; + return result.id; + }) || []; + + for (const action of step.actions) { + for (const output of action.outputs) { + const result = processThing(output, newNodesById, action.participants); + newNodesById = result.newNodesById; + + const edgesForThisOutput: DisplayObjectEdge[] = inputIds.map( + (inputId): DisplayObjectEdge => ({ + sourceId: inputId, + targetId: result.id, + }), + ); + newEdges = [...newEdges, ...edgesForThisOutput]; + } + } + + // Return the newly created objects instead of the mutated ones + return { newNodesById, newEdges }; } function processThing( thing: ThingT, - nodesById: { [id: string]: DisplayObject }, + inputNodes: NodesById, participants: RoleInTimeT[] = [], - generateId: () => string, -): string { - const id: string = thing.doi || thing.id || generateId(); - if (!(id in nodesById)) { - nodesById[id] = thingToDisplayObject(thing, id, participants); +): { id: string; newNodesById: NodesById } { + const newNodesById: NodesById = { ...inputNodes }; + const id = thing.doi || thing.id || generateId(newNodesById); + + if (!(id in newNodesById)) { + newNodesById[id] = thingToDisplayObject(thing, id, participants); } - return id; + + return { id, newNodesById }; } -interface NameHaver { - name: string; +function generateId(nodesById: NodesById): string { + let idCounter: number = 1; + let newId: string = `n${idCounter}`; + + while (newId in nodesById) { + idCounter++; + newId = `n${idCounter}`; + } + + return newId; } function thingToDisplayObject( @@ -150,6 +174,10 @@ function extractContentUrls(content: ManifestationT[] | undefined) { .filter((url: string | undefined): url is string => url !== undefined); } +interface NameHaver { + name: string; +} + function extractActorNames(participants: RoleInTimeT[]) { return participants .map((participant) => participant.actor) @@ -184,6 +212,7 @@ function formatDate(date: Date) { return yyyy + '-' + mm + '-' + dd; } +// These sort functions are only used by tests currently, but seem universally useful export function sortDisplayObjects(objects: DisplayObject[]): DisplayObject[] { return [...objects].sort((a: DisplayObject, b: DisplayObject) => a.nodeId.localeCompare(b.nodeId), diff --git a/packages/widget/src/docmaps-widget.ts b/packages/widget/src/docmaps-widget.ts index 300de4e0..603da357 100644 --- a/packages/widget/src/docmaps-widget.ts +++ b/packages/widget/src/docmaps-widget.ts @@ -1,7 +1,7 @@ import { html, HTMLTemplateResult, LitElement, nothing } from 'lit'; import { customElement, property, state } from 'lit/decorators.js'; import { customCss } from './styles'; -import { closeDetailsButton, logo, renderDetailNavigationHeader } from './assets'; +import { closeDetailsButton, logo } from './assets'; import { Task } from '@lit/task'; import { DocmapFetchingParams, getDocmap } from './docmap-controller'; import { @@ -12,6 +12,7 @@ import { TYPE_DISPLAY_OPTIONS, } from './util'; import { clearGraph, displayGraph } from './graph-view'; +import { renderDetailNavigationHeader } from './detail-navigation-header'; @customElement('docmaps-widget') export class DocmapsWidget extends LitElement { diff --git a/packages/widget/test/unit/docmap-controller.test.ts b/packages/widget/test/unit/docmap-controller.test.ts index 49547863..8db91360 100644 --- a/packages/widget/test/unit/docmap-controller.test.ts +++ b/packages/widget/test/unit/docmap-controller.test.ts @@ -4,9 +4,9 @@ import { sortDisplayObjectEdges, sortDisplayObjects, stepsToGraph, -} from '../../src/docmap-controller'; -import docmapWithOneStep from '../fixtures/sciety-docmap-1'; -import anotherDocmapWithOneStep from '../fixtures/sciety-docmap-2'; +} from '../../src'; +import smallDocmapWithOneStep from '../fixtures/sciety-docmap-1'; +import largeDocmapWithOneStep from '../fixtures/sciety-docmap-2'; import docmapWithMultipleSteps from '../fixtures/elife-docmap-1'; interface Item { @@ -15,7 +15,7 @@ interface Item { } test('getSteps when there is only one step', (t) => { - const steps = getSteps(docmapWithOneStep); + const steps = getSteps(smallDocmapWithOneStep); t.is(steps.length, 1); const step = steps[0]; t.is(step.inputs[0].doi, '10.21203/rs.3.rs-1043992/v1'); @@ -70,17 +70,17 @@ test('getSteps on non-docmaps', (t) => { }); test('getSteps on docmaps without a first step', (t) => { - const result = getSteps({ ...docmapWithOneStep, 'first-step': undefined }); + const result = getSteps({ ...smallDocmapWithOneStep, 'first-step': undefined }); t.deepEqual(result, []); }); test('getSteps on docmaps with no steps', (t) => { - const result = getSteps({ ...docmapWithOneStep, steps: undefined }); + const result = getSteps({ ...smallDocmapWithOneStep, steps: undefined }); t.deepEqual(result, []); }); test('stepsToGraph for a docmap with one step', (t) => { - const steps = getSteps(docmapWithOneStep); + const steps = getSteps(smallDocmapWithOneStep); const { nodes, edges } = stepsToGraph(steps); t.is(nodes.length, 2); @@ -105,8 +105,8 @@ test('stepsToGraph for a docmap with one step', (t) => { ]); }); -test('stepsToGraph for another docmap with one step', (t) => { - const steps = getSteps(anotherDocmapWithOneStep); +test('stepsToGraph for a larger docmap with one step', (t) => { + const steps = getSteps(largeDocmapWithOneStep); const { nodes, edges } = stepsToGraph(steps); t.is(nodes.length, 4);