Skip to content

Conversation

@coolreader18
Copy link
Collaborator

@coolreader18 coolreader18 commented Nov 26, 2025

Description of Changes

A few main goals here:

  • have our iterator functions return an Iterator object so that users can use its combinators like filter() and find() and reduce(). It's a very new JS api but we happen to know that the module code will always be run in an environment that has it* :)

  • improve lifecycle handling for iterator handles - mainly, if an iterator is not run to completion, it will now eventually get garbage collected, whereas before we would have a resource leak.

It turns out that the easiest way to do both of those things was to turn TableIterator into a generator function, which also happens to make the code much easier to read. Hooray :)

* I did mention it in table_cache (which isn't run in our module host) but it's fine, since that's only in the type system and IteratorObject is defined in typescript's lib.es2015.iterable.d.ts but is only given fancy methods in lib.esnext.iterator.d.ts - so if the user uses esnext, they'll have access to them, but otherwise not.

Expected complexity level and risk

1: this better separates concerns and makes the code clearer in its purpose.

Testing

  • Refactor, so automated testing is sufficient.

@coolreader18 coolreader18 force-pushed the noa/ts-iterator-improvements branch from 1444f05 to 1bfebb8 Compare November 26, 2025 19:54
@bfops bfops added the release-any To be landed in any release window label Dec 1, 2025
@coolreader18 coolreader18 force-pushed the noa/ts-iterator-improvements branch 2 times, most recently from 0194246 to c92631a Compare December 11, 2025 21:10
Copy link
Contributor

@cloutiertyler cloutiertyler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only makes typing changes on the SDK side and we control the server side environment, so I'm cool with this. I also met with Noa to discuss.

@coolreader18 coolreader18 added this pull request to the merge queue Dec 15, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 15, 2025
@coolreader18 coolreader18 added this pull request to the merge queue Dec 15, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 15, 2025
@coolreader18 coolreader18 force-pushed the noa/ts-iterator-improvements branch from c92631a to 992b581 Compare December 16, 2025 18:58
@coolreader18 coolreader18 force-pushed the noa/ts-iterator-improvements branch from 992b581 to 40eeab5 Compare December 17, 2025 20:39
@coolreader18 coolreader18 added this pull request to the merge queue Dec 17, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 17, 2025
@coolreader18 coolreader18 added this pull request to the merge queue Dec 18, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 18, 2025
@coolreader18 coolreader18 added this pull request to the merge queue Dec 18, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 18, 2025
@bfops bfops added this pull request to the merge queue Dec 18, 2025
Merged via the queue into master with commit 66f5547 Dec 18, 2025
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-any To be landed in any release window

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants