Skip to content

Extract Graph.js helpers into typed modules - #1819

Open
david-yz-liu wants to merge 5 commits into
masterfrom
refactor-graph-helpers-to-typescript
Open

david-yz-liu wants to merge 5 commits into
masterfrom
refactor-graph-helpers-to-typescript

Conversation

@david-yz-liu

@david-yz-liu david-yz-liu commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor

Proposed Changes

Pulls the self-contained, non-stateful pieces of Graph.js out into their own files and converts them to TypeScript, as prep for eventually converting Graph.js itself. No behavior changes.

  • loadGraphData.ts — the fetch-response parsing logic from getGraph(), now a pure function instead of a class method
  • util.ts — populateHybridRelatives, findRelationship, numCredits, transformPoint, and formatTransform, all already-pure helpers that no longer need to live on the class
  • requirementParser.ts — moved from js/util/util.ts into the graph component directory, since it's only ever used by graph code
  • Made GraphNodeJSON.id_ required instead of optional, after fixing Bool.tsx's test fixture to use id_ (matching real bool-node data, which always has it) instead of a stray id field — this let Node.tsx, util.ts, and loadGraphData.ts drop their & { id_: string } workaround
  • Minor docstring and explicit-parameter-type cleanup in util.ts/loadGraphData.ts

Graph.js drops from 1832 to about 1550 lines. GenerateForm.js and the existing unit tests for the extracted functions were updated for the new import paths; Bool.test.js's snapshot was updated to match (it now shows id_="bool1", which is what the component actually renders).

One TypeScript wrinkle: .flat(Infinity) on the recursive PrereqTree type blew up the compiler ("type instantiation is excessively deep"), a known limitation with self-referential array types in .flat()'s typing. Worked around by casting through unknown[] before flattening — no behavior change.

Screenshots of your changes (if applicable)

Not applicable — no UI changes.

Type of Change

Type Applies?
🚨 Breaking change (fix or feature that would cause existing functionality to change)
✨ New feature (non-breaking change that adds functionality)
🐛 Bug fix (non-breaking change that fixes an issue)
🎨 User interface change (change to user interface; provide screenshots)
♻️ Refactoring (internal change to codebase, without changing functionality) X
🚦 Test update (change that only adds or modifies tests)
📦 Dependency update (change that updates a dependency)
🔧 Internal (change that only affects developers or continuous integration)

Checklist

Before opening your pull request:

  • I have performed a self-review of my changes.
    • Check that all changed files included in this pull request are intentional changes.
    • Check that all changes are relevant to the purpose of this pull request, as described above.
  • I have added tests for my changes, if applicable.
    • This is required for all bug fixes and new features.
    • Not applicable here — pure refactor, existing tests cover the extracted logic (plus one new type guarantee verified by the existing Bool snapshot).
  • I have updated the project documentation, if applicable.
    • This is required for new features.
    • Not applicable — no new features.
  • If this is my first contribution, I have added myself to the list of contributors.
    • Not applicable.
  • I have updated the project Changelog (this is required for all changes).

After opening your pull request:

  • I have verified that the CI checks have passed.
  • I have reviewed the test coverage changes reported by Coveralls.
  • I have requested a review from a project maintainer.

Questions and Comments

None.

🤖 Generated with Claude Code

Pulls the self-contained, non-stateful pieces of Graph.js out into their
own files and converts them to TypeScript, as prep for eventually
converting Graph.js itself:

- loadGraphData.ts: the fetch-response parsing logic from getGraph(),
  now a pure function instead of a class method
- util.ts: populateHybridRelatives, findRelationship, numCredits,
  transformPoint, and formatTransform, all already-pure helpers
- requirementParser.ts: moved from js/util/util.ts into the graph
  component directory, since it's only ever used by graph code

Graph.js drops from 1832 to about 1550 lines and keeps its existing
behavior exactly; GenerateForm.js and the two existing unit tests for
the extracted functions were updated for the new import paths.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@coveralls

coveralls commented Oct 18, 2026 •

Copy link
Copy Markdown

Coverage Report for CI Build 39165335525

Coverage decreased (-0.04%) to 60.343%

Details

  • Coverage decreased (-0.04%) from the base build.
  • Patch coverage: 11 uncovered changes across 3 files (111 of 122 lines covered, 90.98%).
  • No coverage regressions found.

Uncovered Changes

File Changed Covered %
js/components/graph/loadGraphData.ts 91 85 93.41%
js/components/graph/util.ts 26 22 84.62%
js/components/graph/Graph.js 5 4 80.0%

Coverage Regressions

No coverage regressions found.


Coverage Stats

Coverage Status
Relevant Lines: 4302
Covered Lines: 2654
Line Coverage: 61.69%
Relevant Branches: 1001
Covered Branches: 546
Branch Coverage: 54.55%
Branches in Coverage %: Yes
Coverage Strength: 157.08 hits per line

💛 - Coveralls

david-yz-liu and others added 4 commits October 18, 2026 15:11
Bool's test fixture used `id` instead of `id_`, which is why
GraphNodeJSON.id_ had to stay optional and callers needing it
(Node.tsx, util.ts, loadGraphData.ts) used a `& { id_: string }`
intersection as a workaround. Real bool node data always has `id_`
(Graph.js keys bool elements by `boolJSON.id_`), so fix the fixture
instead and make id_ required on the shared type, dropping the
intersections. Also more accurate: Bool spreads its JSON prop onto
the DOM, so the fixture now matches what's actually rendered
(id_="bool1" rather than a plain id attribute).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants