Skip to content

feat(data-structures): stabilize IndexedHeap - #7245

Open
tomas-zijdemans wants to merge 6 commits into
denoland:mainfrom
tomas-zijdemans:stabilize-indexed-heap
Open

feat(data-structures): stabilize IndexedHeap#7245
tomas-zijdemans wants to merge 6 commits into
denoland:mainfrom
tomas-zijdemans:stabilize-indexed-heap

Conversation

@tomas-zijdemans

@tomas-zijdemans tomas-zijdemans commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Promotes IndexedHeap from unstable to stable. The API design was already accepted during the unstable period, so this is the mechanical part:

  • Renamed unstable_indexed_heap.ts to indexed_heap.ts (test file too)
  • Export path is now ./indexed-heap, and mod.ts re-exports the module
  • Dropped the 21 @experimental tags and pointed all 29 JSDoc example imports at the new path

One small change beyond the rename: the NaN rejection error now names the operation that was attempted. push() and the constructor throw Cannot push into IndexedHeap: priority is NaN, set() throws Cannot set priority in IndexedHeap: priority is NaN. Before, all paths said "Cannot set priority".

@bartlomieju

Copy link
Copy Markdown
Member

The mechanics here are right — this mirrors the stabilize Deque precedent (bd051c5) exactly, and the docs and tests are in good shape. 72 tests covering empty-heap operations, duplicate keys, missing keys, NaN on every path, ±Infinity, bigint/tuple priorities, plus type-level and stress tests is more than we usually get at stabilization time.

My hesitation is that stabilization makes a few choices permanent, and I'd like them settled before this lands rather than after. None of these are defects — they're all defensible as-is — but they're the ones we can't take back:

  1. get size() vs length. The stable neighbours BinaryHeap and Deque both expose length. size is defensible here since IndexedHeap is keyed and Map-like, and it matches MultiMap — but it means one module ships both spellings permanently.

  2. Default comparator direction. IndexedHeap defaults to ascend (min-heap) while stable BinaryHeap defaults to descend (max-heap). Two heaps exported from the same mod.ts with opposite defaults is a lasting foot-gun even though it's documented. Worth a deliberate decision that we want this rather than inheriting it.

  3. HeapEntry as a root export. It becomes a @std/data-structures export under a fairly generic name that isn't tied to IndexedHeap. IndexedHeapEntry would age better, and renaming is free right now — after stabilization it isn't.

  4. Soak time. The current API shape landed on 2026-06-30 in cc47b5c, which reworked set() and generic priorities, so it's had roughly three weeks and two releases of exposure. That's on the short side for a shape we're committing to.

I'd be happy with 1 and 2 staying as they are if that's the considered call — I mainly want them to be a decision. 3 seems worth doing regardless, since it costs nothing today.

Nit, non-blocking: the NaN error-message change (naming the operation) is a behavior change riding along in a rename PR. The new wording matches the style guide and I'm not asking you to split it out — just noting it for the reviewer's benefit.

@codecov

codecov Bot commented Jul 30, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.03%. Comparing base (cdb83c6) to head (3c8f93b).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7245      +/-   ##
==========================================
- Coverage   95.03%   95.03%   -0.01%     
==========================================
  Files         618      618              
  Lines       51596    51849     +253     
  Branches     9340     9389      +49     
==========================================
+ Hits        49035    49275     +240     
- Misses       2021     2030       +9     
- Partials      540      544       +4     

☔ View full report in Codecov by Harness.
📢 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.

@tomas-zijdemans

Copy link
Copy Markdown
Contributor Author

Thanks for making these decisions instead of letting them ship by accident. In order:

  1. size stays. The surface is deliberately Map-shaped: set, has, delete, clear, keyed iteration. On the platform, keyed collections spell it size (Map, Set) and sequences spell it length (Array, and our Deque). Renaming size to length while keeping the Map verbs would trade a cross-module inconsistency for an internal one. I'd rather match what the class imitates than its file neighbours.

  2. ascend stays. The workloads an indexed heap exists for (Dijkstra, A*, schedulers, timer queues) all want smaller-value-first, and keyed heaps in most other standard libraries default to min-heap for that reason. BinaryHeap's descend default is the unusual one, inherited from Rust. I've added a sentence to the class doc that names the divergence outright, so nobody discovers it by debugging.

  3. Done. HeapEntry is now IndexedHeapEntry. No alias kept, since it only ever shipped from the unstable path. Agreed this was free today and permanent tomorrow.

  4. Your call. The set() rework has now had a month of exposure. If you'd rather see one more release cycle before stabilizing, I'm happy to let this sit. No urgency on my end.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants