Skip to content

Conversation

@bpowers
Copy link
Owner

@bpowers bpowers commented Jan 28, 2026

Summary

This PR removes the @system-dynamics/xmutil package and integrates Vensim MDL file conversion directly into the engine2 package. The conversion logic is now handled by a new Engine2Project.openVensim() method, simplifying the dependency tree and reducing code duplication.

Key Changes

  • Removed @system-dynamics/xmutil package: Deleted the entire src/xmutil-js directory containing the WASM-based MDL to XMILE converter
  • Updated workspace configuration: Removed xmutil from package.json workspaces and tsconfig.json references
  • Integrated conversion into engine2: Replaced convertMdlToXmile() calls with Engine2Project.openVensim() method
  • Updated example scripts: Modified examples/serialize.mjs, examples/svg.mjs, and scripts/debug-diagram-gen.mjs to use the new API
  • Simplified file handling: Consolidated MDL/XMILE detection logic into conditional calls to either openVensim() or open() methods
  • Updated documentation: Removed xmutil references from CLAUDE.md and website configuration files

Implementation Details

  • The new Engine2Project.openVensim() method handles Vensim MDL file conversion internally
  • All example scripts now use a ternary operator pattern to select the appropriate method based on file extension
  • The debug-diagram-gen.mjs script now uses engine2Project.toXmileString() to extract XMILE content, eliminating the need to track conversion state
  • Path aliases in website configuration were updated to remove xmutil references

https://claude.ai/code/session_01HgDa6MrE8j46eW8AvLDanW

The xmutil-js package contained a stale WASM build of Bob Eberlein's
C++ xmutil tool for converting Vensim MDL to XMILE. This functionality
is now provided natively by the Rust simlin-compat crate, exposed
through engine2's Project.openVensim() API.

Updated the example scripts and debug-diagram-gen to use openVensim()
directly instead of the two-step convert-then-open pattern. Removed
all workspace, tsconfig, and alias references.

https://claude.ai/code/session_01HgDa6MrE8j46eW8AvLDanW
@claude
Copy link

claude bot commented Jan 28, 2026

Pull Request Review: Remove xmutil package and integrate Vensim conversion into engine2

Summary

This PR successfully removes the @system-dynamics/xmutil TypeScript/WASM wrapper package and consolidates Vensim MDL file conversion into the engine2 package via a new Engine2Project.openVensim() method. The change simplifies the dependency tree and reduces code duplication.

Code Quality: ✅ Excellent

Strengths

  1. Clean API consolidation: The new Engine2Project.openVensim() method provides a cleaner, more intuitive API compared to the previous two-step approach (convertMdlToXmile() then open()).

  2. Consistent pattern: The updated usage pattern in example scripts is elegant and follows good TypeScript practices:

    const project = inputFile.endsWith('.mdl')
      ? await Engine2Project.openVensim(contents, { wasm: wasmPath })
      : await Engine2Project.open(contents, { wasm: wasmPath });
  3. Well-implemented public API: The openVensim() method at line 163 in src/engine2/src/project.ts is properly documented, handles both string and Uint8Array inputs, and follows the same pattern as other open* methods.

  4. Smart use of toXmileString(): The debug-diagram-gen.mjs script now uses engine2Project.toXmileString() to extract XMILE content, which is more robust than tracking conversion state manually.

  5. Complete removal: The PR thoroughly removes all references to the @system-dynamics/xmutil package from:

    • Workspace configuration (package.json)
    • TypeScript references (tsconfig.json)
    • Documentation (CLAUDE.md)
    • Build configurations (website/rspress.config.ts, website/tsconfig.json)

Test Coverage: ✅ Good

The openVensim() method has test coverage in src/engine2/tests/api.test.ts:1126-1150, testing both Uint8Array and string inputs.

Potential Improvements

Minor: Error Handling Clarity

In the updated scripts (examples/serialize.mjs, examples/svg.mjs, scripts/debug-diagram-gen.mjs), the file extension check uses a simple string suffix test:

inputFile.endsWith('.mdl')

Consideration: This approach is simple and works well for the current use case. However, if you want to be more robust against edge cases (e.g., files named something.mdl.txt), you could consider using a more explicit check or extracting the extension properly. That said, for these example/debug scripts, the current approach is perfectly reasonable.

Minor: Logging Loss

The old convertMdlToXmile() function returned optional log messages:

const [xmileContents, logs] = await convertMdlToXmile(contents, true);
if (logs) {
  console.log('Conversion logs:', logs);
}

The new openVensim() method doesn't expose these logs. If diagnostic logging was valuable for debugging Vensim conversion issues, this might be worth preserving through:

  • Error messages in thrown SimlinError exceptions
  • A separate method to retrieve conversion diagnostics
  • Or accepting that the Rust layer handles logging internally

Question: Were the conversion logs primarily for debugging, or do users rely on them?

Architecture & Design: ✅ Excellent

  1. Reduced complexity: Eliminating the intermediate TypeScript WASM wrapper reduces the number of WASM modules and simplifies the build pipeline.

  2. Better encapsulation: The Vensim conversion logic is now internal to the engine2 package, which makes more sense architecturally since it's a format conversion concern.

  3. Maintains Rust xmutil: The underlying Rust src/xmutil crate is preserved, showing this is purely a TypeScript-level consolidation, not a removal of the conversion capability itself.

Performance: ✅ Neutral to Positive

  • Removes one WASM module load
  • Simplifies the call chain (no intermediate conversion step)
  • Should have equivalent or slightly better performance

Security: ✅ No Concerns

No security issues identified. The change is a refactoring that doesn't introduce new attack surfaces.

Documentation: ✅ Good

  • CLAUDE.md updated to remove xmutil reference
  • The PR description provides clear context on what changed and why

Compatibility Notes

This is a breaking change for any external code using @system-dynamics/xmutil directly. However, based on the CLAUDE.md guidance:

You should think of this as a monorepo without external users that depend on it – do NOT worry about breaking changes or backwards compatibility

This is appropriate for the project's current state.

Recommendations

Approve with minor observations

This is a well-executed refactoring that simplifies the codebase without sacrificing functionality. The code quality is excellent, test coverage exists, and the documentation is updated appropriately.

Optional Follow-ups (Non-blocking)

  1. Consider whether conversion diagnostic logs should be exposed through the new API
  2. Verify that all CI checks pass (I wasn't able to check this directly)
  3. If there are integration tests that use Vensim MDL files, ensure they're passing

Great work on this cleanup! The new API is more intuitive and the codebase is simpler.

@bpowers bpowers merged commit e3daec4 into main Jan 28, 2026
9 checks passed
@bpowers bpowers deleted the claude/remove-xmutil-js-FiNGd branch January 28, 2026 14:31
@codecov
Copy link

codecov bot commented Jan 28, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.82%. Comparing base (5fa9982) to head (1a7d154).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #218   +/-   ##
=======================================
  Coverage   76.82%   76.82%           
=======================================
  Files          69       69           
  Lines       20523    20523           
=======================================
  Hits        15766    15766           
  Misses       4757     4757           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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.

3 participants