Skip to content

Add Iterator.prototype bindings #7506

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Add Iterator.prototype bindings #7506

wants to merge 12 commits into from

Conversation

nojaf
Copy link
Collaborator

@nojaf nojaf commented May 23, 2025

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 pkg.pr.new
Copy link

pkg-pr-new bot commented May 23, 2025

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/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/darwin-x64

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

@rescript/win32-x64

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

commit: c40bf36

@nojaf nojaf marked this pull request as ready for review May 23, 2025 08:55
mediremi
mediremi reviewed May 23, 2025
View reviewed changes
runtime/Stdlib_Iterator.res
external toArrayWithMapper: (t<'a>, 'a => 'b) => array<'b> = "Array.from"

let forEach = (iterator, f) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

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
cknitt reviewed May 23, 2025
View reviewed changes
Copy link
Member

@cknitt cknitt left a comment

Choose a reason for hiding this comment

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

Great stuff, thanks a lot!

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

@nojaf
Copy link
Collaborator Author

nojaf commented May 23, 2025

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.

cknitt
cknitt reviewed May 24, 2025
View reviewed changes
cknitt
cknitt reviewed May 24, 2025
View reviewed changes
@nojaf
Copy link
Collaborator Author

nojaf commented May 26, 2025

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.

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

@cknitt cknitt cknitt left review comments

@mediremi mediremi mediremi left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nojaf @cknitt @mediremi