Add Iterator.prototype bindings

[nojaf]

This may be a bit controversial, but I've added bindings for Iterators and Generators and corrected some bindings, resulting in a minor breaking change.

[pkg-pr-new]

Open in StackBlitz

rescript

npm i https://pkg.pr.new/rescript-lang/rescript@7506

@rescript/darwin-arm64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/darwin-arm64@7506

@rescript/darwin-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/darwin-x64@7506

@rescript/linux-arm64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/linux-arm64@7506

@rescript/linux-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/linux-x64@7506

@rescript/win32-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/win32-x64@7506

commit: c394634

[mediremi]

I'm for this change 👍

Iterator.forEach's behaviour was different to Iterator.prototype.forEach as the latter only calls its callback with yielded values, not the iterator's final return value.

[cknitt]

Great stuff, thanks a lot!

But could you use tests (using assertEqual) instead of doing Console.logs in the doc strings?

[nojaf]

But could you use tests (using assertEqual) instead of doing Console.logs in the doc strings?

Yep, will do! I'm less familiar with that part of the codebase.

[nojaf]

This one seems to fail on Linux:

rescript/tests/tests/src/js_re_test.res

Lines 5 to 19 in 97a81e5

	list{
	(
	"captures",
	_ => {
	let re = /(\d+)-(?:(\d+))?/g
	let str = "3-"
	switch re->Js.Re.exec_(str) {
	| Some(result) =>
	let defined = Js.Re.captures(result)[1]
	let undefined = Js.Re.captures(result)[2]
	Eq((Js.Nullable.return("3"), Js.Nullable.null), (defined, undefined))
	| None => Fail()
	}
	},
	),

No clue why, or how it could be related to my changes.

[fhammerschmidt]

As suggested by @shulhi we should just add docstrings with information about the JS runtimes that are required to use this.

[nojaf]

I'm tempted to remove external toArrayWithMapper: (t<'a>, 'a => 'b) => array<'b> = "Array.from" as well here.
I've added https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Iterator/toArray

We can stick to the actual MDN-listed method names instead of an alias.
@cknitt you added toArrayWithMapper originally, what do you think?

[cknitt]

I'm tempted to remove external toArrayWithMapper: (t<'a>, 'a => 'b) => array<'b> = "Array.from" as well here. I've added https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Iterator/toArray

We can stick to the actual MDN-listed method names instead of an alias. @cknitt you added toArrayWithMapper originally, what do you think?

I don't think I added it originally, I just moved it from the Core repo to the compiler repo.

I have no issue with removing it as long as you add a deprecation message explaining what to use instead.

[nojaf]

While looking at the replacement, I noticed it is not an exact fit.
I want to leave this as is.

@zth and @aspeddro, I noticed you originally added Iterators in ReScriptCore.

Could someone review, please?

[zth]

This looks good to me, but I don't have strong opinions on this. So if people think this is fine then I'm also fine with it.

[cristianoc]

@nojaf

Can't do make test locally anymore after this:

file:///Users/cristianocalcagno/GitHub/rescript-compiler/tests/tests/src/core/Core_IteratorTests.mjs:19
iterator.forEach(v => {
         ^

TypeError: iterator.forEach is not a function
    at file:///Users/cristianocalcagno/GitHub/rescript-compiler/tests/tests/src/core/Core_IteratorTests.mjs:19:10
    at ModuleJob.run (node:internal/modules/esm/module_job:263:25)
    at async ModuleLoader.import (node:internal/modules/esm/loader:540:24)
    at async asyncRunEntryPointWithESMLoader (node:internal/modules/run_main:117:5)

Node.js v20.19.0

file:///Users/cristianocalcagno/GitHub/rescript-compiler/tests/tests/src/core/Core_IteratorTests.mjs:19
iterator.forEach(v => {
^

TypeError: iterator.forEach is not a function
at file:///Users/cristianocalcagno/GitHub/rescript-compiler/tests/tests/src/core/Core_IteratorTests.mjs:19:10
at ModuleJob.run (node:internal/modules/esm/module_job:263:25)
at async ModuleLoader.import (node:internal/modules/esm/loader:540:24)
at async asyncRunEntryPointWithESMLoader (node:internal/modules/run_main:117:5)

Node.js v20.19.0

And node `v20.x` I believe is the recommended version but this might require a newer version?

[mediremi]

@cristianoc to run the unit tests, you'll need to update to node.js v22, which supports Iterator.prototype.forEach.

[cristianoc]

looks like CONTRIBUTING.md needs updating

[mediremi]

I've made a PR for updating CONTRIBUTING.md here: #7558