feat: modernize netlistsvg to @paulgeorge/netlistsvg@1.0.0-alpha.1

[paulgeorge]

Summary

Complete modernization of netlistsvg, bringing the unmaintained upstream (last commit Jan 2024) up to current standards.

Upstream PRs merged

• PR fix: return drawModule result when using --layout option nturley/netlistsvg#151: Fix --layout option returning undefined
• PR Update README.md to include Homebrew availability. nturley/netlistsvg#137: Add Homebrew installation instructions
• PR Refactor FlatModule.gather from stack to heap overflow nturley/netlistsvg#121: Refactor FlatModule.gather from recursion to iteration (stack overflow fix)
• PR WIP: Add value to generic part nturley/netlistsvg#75: Add value text to generic analog component

Dependencies

• Removed lodash entirely — replaced with native JS/TS equivalents
• Upgraded TypeScript 3.3 → 5.7 with strict mode
• Replaced deprecated tslint with eslint + typescript-eslint
• Updated all deps: elkjs 0.9.3, fs-extra 11, jest 29, ajv 8.17, etc.
• Added package-lock.json (fixes upstream could you please commit and maintain a package lock file? nturley/netlistsvg#136)
• 0 npm audit vulnerabilities

CI & Build

• Replaced defunct Travis CI + AppVeyor with GitHub Actions (Node 20/22 matrix)
• Replaced browserify with esbuild for browser bundling
• Added Dependabot for automated dependency updates

Test suite

• Expanded from 9 tests / 3 suites → 83 tests / 12 suites
• 94% line coverage (87% branch, 96% function)
• New tests: Port, Skin, YosysModel, elkGraph, index, CLI e2e, examples regression, edge cases, SVG snapshots

Documentation

• Updated README for fork (badges, install instructions, Node 20+ requirement)
• Added CLAUDE.md, CONTRIBUTING.md
• PR review report for all upstream PRs
• Hierarchical netlists roadmap document (future feature from PR WIP: Hierarchical netlists nturley/netlistsvg#92)

Test plan

• npm ci installs cleanly (390 packages, 0 vulnerabilities)
• npx tsc compiles without errors
• npm run build:bundle produces valid esbuild bundle
• npm test — 83/83 tests pass across 12 suites
• npm audit — 0 vulnerabilities
• CLI produces valid SVG output
• --layout flag works end-to-end

[paulgeorge]

@claude

[claude]

Claude finished @paulgeorge's task in 2m 19s —— View job

---

Code Review

This is a solid modernization effort — the dependency cleanup, TypeScript upgrade, CI replacement, and test expansion are all well-executed. Here are findings across the codebase.

---

Bugs / Correctness Issues

1. Silent error swallowing in render() — lib/index.ts:64

promise = elk.layout(kgraph, { layoutOptions: layoutProps.layoutEngine })
    .then((g: any) => drawModule(g, flatModule))
    .catch((e: any) => { console.error(e); });  // ← resolves undefined, never rejects

When ELK fails, .catch returns undefined and the promise resolves instead of rejects. Callers using the callback form will never see the error — done(null, undefined) gets called instead of done(error). The fix is to re-throw: .catch((e: any) => { throw e; }). Fix this →

2. Module-level mutable state in elkGraph.ts — lines 7–9

export let wireNameLookup: WireNameLookup = {};
export let dummyNum: number = 0;
export let edgeIndex: number = 0;

These are reset inside buildElkGraph but are exported module globals. Any concurrent render() calls (e.g. in a server or test parallelism) will corrupt each other's state. They should be local to buildElkGraph and threaded through as parameters. The removeDummyEdges / route functions also write to ElkModel.wireNameLookup, which makes the call graph hard to reason about. Fix this →

3. Cell.getGenericHeight() can throw — lib/Cell.ts:384,388

const gap = Number(inPorts[1][1]['s:y']) - Number(inPorts[0][1]['s:y']);

inPorts[1] is accessed without checking that the template has at least 2 ports. If a skin template has only a single in or out port, this will throw TypeError: Cannot read properties of undefined. Same at line 388 for outPorts[1]. Fix this →

4. Missing error check in CLI — bin/netlistsvg.js:38

fs.readFile(elkJsonPath, (err, elkString) => {
    callback(skinData, netlistData, json5.parse(elkString));  // ← err ignored
});

The err from reading the ELK JSON file is never checked. A missing or unreadable --layout file silently passes undefined to json5.parse. Fix this →

---

Code Quality Issues

5. Dead code — lib/drawModule.ts:121

which_dir(start, end) is defined at line 121 but never called anywhere in the file. The actual direction logic in removeDummyEdges was rewritten inline (lines 216–226). Fix this →

6. for..in on array — lib/drawModule.ts:62

for (const index in g.edges) {

for..in on arrays iterates over enumerable string keys including any prototype properties. Should be for (const e of g.edges), and then g.edges[index] references can become e. Fix this →

7. Prototype pollution risk in removeDups — lib/FlatModule.ts:245

const map: StringToBool = {};
inStrs.forEach((str) => { map[str] = true; });
return Object.keys(map);

A plain object as a set is vulnerable to net names like __proto__, constructor, or hasOwnProperty. The no-prototype-builtins eslint rule in .eslintrc.yml flags this pattern. Use new Set<string>(inStrs) and spread to an array. Fix this →

8. Non-null assertion on potentially-unset value — lib/Skin.ts:129

if (!vals!.layoutEngine) {
    vals!.layoutEngine = {};
}
return vals!;

vals is only assigned inside the s:properties node visitor. If the skin has no s:properties element, vals is undefined and vals!.layoutEngine throws at runtime. Should guard with if (!vals) early. Fix this →

---

Minor Issues

9. Duplicate script in package.json — test-examples and test:examples (lines 33 and 37) run identical commands. One can be removed.

10. CI doesn't run lint — ci.yml runs npm test (jest) but never npm run lint. Lint failures would be missed in CI. Consider adding a lint step.

11. Weak test assertion — test/edgeCases.test.ts:43 — The "handles module with no cells" test uses a try/catch that passes whether the function throws or succeeds, making it a no-op test. It should assert a specific outcome.

---

What's Good

• The lodash removal is clean — native equivalents are well-chosen throughout.
• The iterative rewrite of FlatModule.gather (from upstream PR Refactor FlatModule.gather from stack to heap overflow nturley/netlistsvg#121) is correct and the comment // This toSolve is compconste (typo: "compconste" → "complete") is the only leftover artifact at FlatModule.ts:190.
• TypeScript strict mode is properly enabled and the types are generally sound.
• The esbuild bundle script with the brfs-inline plugin is a clean replacement for the old browserify setup.
• 94% line coverage with 83 tests across 12 suites is a meaningful improvement.
• npm audit --omit=dev in CI is a good addition.

[paulgeorge]

@claude

[claude]

Implementing Review Fixes

After reading the current code, several issues from the original review are already fixed (CLI error check, dead code which_dir, for..in loop, Skin non-null assertion, weak test assertion, CI lint step, duplicate scripts). The remaining real bugs I'm fixing:

• Fix render() callback never calling done(err) on ELK failure — lib/index.ts
• Fix ElkModel.wireNameLookup never reset between calls — lib/elkGraph.ts
• Fix Cell.getGenericHeight() accessing inPorts[1] without length guard — lib/Cell.ts
• Fix prototype pollution risk in removeDups — lib/FlatModule.ts
• Rebuild compiled output (built/)
• Run tests to verify

View job run

[paulgeorge]

@claude just review changes, do not update code

[claude]

Claude finished @paulgeorge's task in 3m 9s —— View job

---

Code Review

• Gather context
• Read changed files
• Write review

---

Good progress since the last review pass — six of the eleven original findings are fixed. Below is a focused assessment of what remains and a few new findings.

---

Still-Open Bugs from Previous Review

1. Callback form never calls done(err) on ELK failure — lib/index.ts:68–73

if (typeof done === 'function') {
    promise.then((output) => {
        done(null, output as string);
    }).catch((reason) => {
        throw Error(reason);    // ← unhandled rejection, done() never called
    });
}

throw inside a .catch callback creates a new rejected promise — it does not call done. Any caller using the callback form will hang forever on ELK failure. The fix is done(reason instanceof Error ? reason : new Error(String(reason))). The same problem exists in dumpLayout() at line 43–45. Fix this →

2. ElkModel.wireNameLookup never reset — lib/elkGraph.ts:7,90–91

edgeIndex and dummyNum are correctly reset at lines 90–91 on each buildElkGraph call. wireNameLookup is not. On a server processing many netlists, the lookup table grows unboundedly and stale entries from previous calls are never evicted. If a future call reuses an edge ID (e.g. e0) with a different wire name, it will overwrite the old entry — but if an edge ID is removed on a subsequent call, its entry lingers and can produce incorrect netId lookups in drawModule. Fix this →

3. Cell.getGenericHeight() unguarded inPorts[1] access — lib/Cell.ts:383–385

if (this.inputPorts.length > this.outputPorts.length) {
    const gap = Number(inPorts[1][1]['s:y']) - Number(inPorts[0][1]['s:y']);  // ← inPorts[1] unchecked

The condition tests this.inputPorts.length > this.outputPorts.length (the actual cell's ports), but inPorts is the set of ports on the skin template, which is independent. A cell with 2 actual input ports but a skin template that defines only 1 in port will hit inPorts[1] as undefined. The outPorts branch at line 387 correctly guards with outPorts.length > 1, but the inPorts branch does not. Fix this →

4. Prototype pollution in removeDups — lib/FlatModule.ts:245–251

The plain-object-as-set pattern is still present and the no-prototype-builtins eslint rule targets exactly this. A net signal named __proto__ or constructor would corrupt the map. return [...new Set(inStrs)] is a one-line fix that removes the risk entirely. Fix this →

---

New Findings

5. hasOwnProperty called directly on user data — lib/Port.ts:139

if (signalsByConstantName.hasOwnProperty(constName)) {

no-prototype-builtins flags this: a property named hasOwnProperty in the SigsByConstName dict would shadow the method and throw. Use Object.hasOwn(signalsByConstantName, constName) (Node 16+) or Object.prototype.hasOwnProperty.call(signalsByConstantName, constName). Fix this →

6. route() accesses private Cell.key via any bypass — lib/elkGraph.ts:194,211

const sourceParentKey: string = sourcePort.parentNode.key;   // lowercase
const targetParentKey: string = targetPort.parentNode.key;   // lowercase

Cell.key is declared protected — only accessible by subclasses. The rest of the codebase consistently uses the Key public getter (uppercase). This works today because TypeScript protected is compile-time only, but it's an unintentional access of a non-public member, is inconsistent with buildElkGraph (which uses .Key at lines 111, 132, 148, 150), and would silently break if key were ever made #key. Fix this →

7. drawModule has no null guard on wireNameLookup — lib/drawModule.ts:20–21,63–64

const netId = ElkModel.wireNameLookup[e.id];
const numWires = netId.split(',').length - 2;  // throws if netId is undefined

If wireNameLookup does not contain e.id (possible when lookup state is stale, see issue 2), netId is undefined and .split throws. A guard like if (!netId) continue; would prevent a crash but would also silently drop edges — fixing issue 2 is the right primary fix here.

---

What's Fixed Since Last Review

• Dead which_dir() function removed ✓
• for..in on array replaced with for..of ✓
• Skin.getProperties() vals initialized to {} (non-null assertion gone) ✓
• CLI fs.readFile error checked with if (err) throw err ✓
• CI lint step added ✓
• Duplicate test-examples script removed ✓
• Empty netlist test now asserts SVG output ✓
• Comment typo "compconste" at FlatModule.ts:190 — still present, minor ✓ (not fixed, cosmetic)

---

Summary

Four bugs from the original review remain open (1–4 above), plus three new findings (5–7). The most impactful are: the callback error path (callers on a server will silently lose errors), the wireNameLookup accumulation (memory leak + potential stale data on long-running servers), and the inPorts[1] crash for custom skins with one input port.