Skip to content

Conversation

@sequba
Copy link
Contributor

@sequba sequba commented Jan 9, 2026

Context

Replaced the internal nodesIds: Map<Node, NodeId> in the Graph class with a direct idInGraph property stored on each node. This eliminates the Map that could exceed maximum size limits when loading large spreadsheets with many cells.

Co-authored by @Krizz (#1597)

Changes:

  • Added GraphNode interface with idInGraph property
  • Created Vertex abstract base class implementing GraphNode
  • Created CellVertex abstract class extending Vertex for cell-type vertices
  • Updated all vertex classes to extend the appropriate base class:
  • Removed nodesIds Map from Graph class
  • Removed getNodeId() public method (now access node.idInGraph directly)
  • Updated getInfiniteRanges() return type from NodeAndId<Node>[] to Node[]

How did you test your changes?

Types of changes

  • Breaking change (a fix or a feature because of which an existing functionality doesn't work as expected anymore)
  • New feature or improvement (a non-breaking change that adds functionality)
  • Bug fix (a non-breaking change that fixes an issue)
  • Additional language file, or a change to an existing language file (translations)
  • Change to the documentation

Related issues:

  1. Fixes Error Map maximum size exceeded when loading a 30-milion-cell worksheet #1602

Checklist:

  • I have reviewed the guidelines about Contributing to HyperFormula and I confirm that my code follows the code style of this project.
  • I have signed the Contributor License Agreement.
  • My change is compliant with the OpenDocument standard.
  • My change is compatible with Microsoft Excel.
  • My change is compatible with Google Sheets.
  • I described my changes in the CHANGELOG.md file.
  • My changes require a documentation update.
  • My changes require a migration guide.

Krizz and others added 3 commits January 8, 2026 12:10
* remove nodeids map and store node id directly

* add _graphId property

---------

Co-authored-by: Kuba Sekowski <[email protected]>
@github-actions
Copy link

github-actions bot commented Jan 9, 2026

Performance comparison of head (5cdf351) vs base (f706d17)

                                     testName |   base |   head | change
------------------------------------------------------------------------
                                      Sheet A | 507.89 | 492.17 | -3.10%
                                      Sheet B |  162.8 | 158.83 | -2.44%
                                      Sheet T |  141.7 | 136.33 | -3.79%
                                Column ranges | 499.85 | 464.49 | -7.07%
Sheet A:  change value, add/remove row/column |  14.34 |  15.69 | +9.41%
 Sheet B: change value, add/remove row/column | 127.63 | 133.24 | +4.40%
                   Column ranges - add column | 143.23 | 148.12 | +3.41%
                Column ranges - without batch | 429.13 | 443.21 | +3.28%
                        Column ranges - batch | 114.88 | 110.32 | -3.97%

@sequba sequba marked this pull request as ready for review January 9, 2026 13:50
@sequba sequba requested a review from budnix January 9, 2026 14:22
@codecov
Copy link

codecov bot commented Jan 9, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.18%. Comparing base (f706d17) to head (5cdf351).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #1604   +/-   ##
========================================
  Coverage    97.17%   97.18%           
========================================
  Files          170      172    +2     
  Lines        14785    14800   +15     
  Branches      3245     3246    +1     
========================================
+ Hits         14368    14383   +15     
  Misses         409      409           
  Partials         8        8           
Files with missing lines Coverage Δ
...c/DependencyGraph/AddressMapping/AddressMapping.ts 93.70% <ø> (ø)
...rc/DependencyGraph/AddressMapping/DenseStrategy.ts 100.00% <ø> (ø)
...c/DependencyGraph/AddressMapping/SparseStrategy.ts 80.18% <ø> (ø)
src/DependencyGraph/CellVertex.ts 100.00% <100.00%> (ø)
src/DependencyGraph/DependencyGraph.ts 98.81% <100.00%> (ø)
src/DependencyGraph/EmptyCellVertex.ts 100.00% <100.00%> (ø)
src/DependencyGraph/FormulaVertex.ts 84.25% <100.00%> (+0.25%) ⬆️
src/DependencyGraph/Graph.ts 99.30% <100.00%> (-0.01%) ⬇️
src/DependencyGraph/ParsingErrorVertex.ts 100.00% <100.00%> (ø)
src/DependencyGraph/RangeVertex.ts 100.00% <100.00%> (ø)
... and 4 more
🚀 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