Skip to content
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.

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

Spec the forEach() operator #105

Merged
merged 3 commits into from
Feb 7, 2024
Merged

Spec the forEach() operator #105

merged 3 commits into from
Feb 7, 2024

Conversation

domfarolino
Copy link
Collaborator

@domfarolino domfarolino commented Jan 30, 2024

Prototype Chromium implementation (with WPTs): https://chromium-review.googlesource.com/c/chromium/src/+/5249869.


Preview | Diff

@domfarolino domfarolino requested a review from benlesh January 30, 2024 21:57
@domfarolino domfarolino marked this pull request as ready for review January 30, 2024 21:59
@domfarolino domfarolino requested a review from keithamus January 30, 2024 22:10
@domfarolino
Copy link
Collaborator Author

Feel free to take a look @keithamus, if you're interested. But no pressure.

Copy link
Collaborator

@keithamus keithamus left a comment

Choose a reason for hiding this comment

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

This one LGTM 👍

@@ -743,7 +743,76 @@ For now, see [https://github.com/wicg/observable#operators](https://github.com/w
<div algorithm>
The <dfn for=Observable method><code>forEach(|callback|, |options|)</code></dfn> method steps are:

1. <span class=XXX>TODO: Spec this and use |callback| and |options|.</span>
1. Let |p| [=a new promise=].
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems generally useful but I'm curious if there was any discussion weighing up why this should return a promise, vs undefined per Array#forEach, or if this should perhaps be renamed to differentiate itself from any symmetry of the array equivalent?

Copy link
Collaborator Author

@domfarolino domfarolino Jan 30, 2024

Choose a reason for hiding this comment

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

I think the analogy is that this kind of since array iteration is synchronous and returns undefined when done, therefore Observable "iteration" (i.e., forEach()) would return a Promise<undefined>, since that will tell you when Observable "iteration" is is done (since it is asynchronous). That kind of timing information could be useful.

There is some discussion about this on #69, but basically I'm not sure there is much utility in any operator returning undefined ever, vs Promise<undefined> which carries strictly more information. But I'd be open to re-consideration if you want to strike up more conversation or file another issue.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't want to rock the boat on what is likely well-thought out and a culmination of very long discussions, but having said that, part of me wonders if this is a little awkward and maybe it's just... not worth implementing forEach at all? I wonder how much value it adds. Presumably given how much of this is modeled on existing user-land libraries we can find some kind of usage figures to guide us on which API should be prioritized? Is forEach high in that list?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm sure you have an answer for the above so I filed #106 to hopefully document that!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As an individual operator, yeah I think forEach() is useful. If we don't provide it, people will basically just try and use map() in a non-idiomatic way to do the same thing, like this example which I think is an accurate portrayal of what would come, absent this operator: whatwg/dom#544 (comment).

Secondary to the utility, it provides parity with the Iterator helpers proposal which I think kinda backs up the utility argument, and makes things a bit more symmetrical.

This operator is kinda somewhat similar to https://rxjs.dev/api/operators/tap which is a really popular operator, although admittedly it is different in that it doesn't return an Observable. But you brought this point up in 2019 anyways, which seems reasonable.

In short, I think the operator is useful, however I'm open to discussing what it should return. The existing momentum points to Promise<undefined> but if we want to have further discussion I'm fine with that too.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jan 30, 2024
This CL implements the semantics specified in
https://wicg.github.io/observable/#dom-observable-foreach.

See WICG/observable#105.

For WPTs:
Co-authored-by: [email protected]

[email protected]

Bug: 1485981
Change-Id: I61344bad7fa4bac65146e1305a376fc1f5e55dc3
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Feb 6, 2024
This CL implements the semantics specified in
https://wicg.github.io/observable/#dom-observable-foreach.

See WICG/observable#105.

For WPTs:
Co-authored-by: [email protected]

[email protected]

Bug: 1485981
Change-Id: I61344bad7fa4bac65146e1305a376fc1f5e55dc3
@domfarolino
Copy link
Collaborator Author

The Chromium implementation for this has landed with tests, so I'll merge this.

@domfarolino domfarolino merged commit 35b0376 into master Feb 7, 2024
2 checks passed
@domfarolino domfarolino deleted the forEach-operator branch February 7, 2024 14:52
github-actions bot added a commit that referenced this pull request Feb 7, 2024
SHA: 35b0376
Reason: push, by domfarolino

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Feb 7, 2024
This CL implements the semantics specified in
https://wicg.github.io/observable/#dom-observable-foreach.

See WICG/observable#105.

For WPTs:
Co-authored-by: [email protected]

[email protected]

Bug: 1485981
Change-Id: I61344bad7fa4bac65146e1305a376fc1f5e55dc3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5249869
Reviewed-by: Mason Freed <[email protected]>
Commit-Queue: Dominic Farolino <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1257328}
aarongable pushed a commit to chromium/chromium that referenced this pull request Feb 7, 2024
This CL implements the semantics specified in
https://wicg.github.io/observable/#dom-observable-foreach.

See WICG/observable#105.

For WPTs:
Co-authored-by: [email protected]

[email protected]

Bug: 1485981
Change-Id: I61344bad7fa4bac65146e1305a376fc1f5e55dc3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5249869
Reviewed-by: Mason Freed <[email protected]>
Commit-Queue: Dominic Farolino <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1257328}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Feb 7, 2024
This CL implements the semantics specified in
https://wicg.github.io/observable/#dom-observable-foreach.

See WICG/observable#105.

For WPTs:
Co-authored-by: [email protected]

[email protected]

Bug: 1485981
Change-Id: I61344bad7fa4bac65146e1305a376fc1f5e55dc3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5249869
Reviewed-by: Mason Freed <[email protected]>
Commit-Queue: Dominic Farolino <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1257328}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Feb 13, 2024
…e operator, a=testonly

Automatic update from web-platform-tests
DOM: Implement the `forEach()` Observable operator

This CL implements the semantics specified in
https://wicg.github.io/observable/#dom-observable-foreach.

See WICG/observable#105.

For WPTs:
Co-authored-by: [email protected]

[email protected]

Bug: 1485981
Change-Id: I61344bad7fa4bac65146e1305a376fc1f5e55dc3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5249869
Reviewed-by: Mason Freed <[email protected]>
Commit-Queue: Dominic Farolino <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1257328}

--

wpt-commits: 910e4c850e0e43a1832f943e2782ffa8f06b3eb9
wpt-pr: 44300
jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Feb 15, 2024
…e operator, a=testonly

Automatic update from web-platform-tests
DOM: Implement the `forEach()` Observable operator

This CL implements the semantics specified in
https://wicg.github.io/observable/#dom-observable-foreach.

See WICG/observable#105.

For WPTs:
Co-authored-by: [email protected]

[email protected]

Bug: 1485981
Change-Id: I61344bad7fa4bac65146e1305a376fc1f5e55dc3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5249869
Reviewed-by: Mason Freed <[email protected]>
Commit-Queue: Dominic Farolino <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1257328}

--

wpt-commits: 910e4c850e0e43a1832f943e2782ffa8f06b3eb9
wpt-pr: 44300
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Feb 16, 2024
…e operator, a=testonly

Automatic update from web-platform-tests
DOM: Implement the `forEach()` Observable operator

This CL implements the semantics specified in
https://wicg.github.io/observable/#dom-observable-foreach.

See WICG/observable#105.

For WPTs:
Co-authored-by: benbenlesh.com

R=masonfchromium.org

Bug: 1485981
Change-Id: I61344bad7fa4bac65146e1305a376fc1f5e55dc3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5249869
Reviewed-by: Mason Freed <masonfchromium.org>
Commit-Queue: Dominic Farolino <domchromium.org>
Cr-Commit-Position: refs/heads/main{#1257328}

--

wpt-commits: 910e4c850e0e43a1832f943e2782ffa8f06b3eb9
wpt-pr: 44300

UltraBlame original commit: b7468884afeb181ab7f2ec3224e73d56c0091360
mbrodesser-Igalia pushed a commit to mbrodesser-Igalia/wpt that referenced this pull request Feb 19, 2024
This CL implements the semantics specified in
https://wicg.github.io/observable/#dom-observable-foreach.

See WICG/observable#105.

For WPTs:
Co-authored-by: [email protected]

[email protected]

Bug: 1485981
Change-Id: I61344bad7fa4bac65146e1305a376fc1f5e55dc3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5249869
Reviewed-by: Mason Freed <[email protected]>
Commit-Queue: Dominic Farolino <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1257328}
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Feb 20, 2024
…e operator, a=testonly

Automatic update from web-platform-tests
DOM: Implement the `forEach()` Observable operator

This CL implements the semantics specified in
https://wicg.github.io/observable/#dom-observable-foreach.

See WICG/observable#105.

For WPTs:
Co-authored-by: benbenlesh.com

R=masonfchromium.org

Bug: 1485981
Change-Id: I61344bad7fa4bac65146e1305a376fc1f5e55dc3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5249869
Reviewed-by: Mason Freed <masonfchromium.org>
Commit-Queue: Dominic Farolino <domchromium.org>
Cr-Commit-Position: refs/heads/main{#1257328}

--

wpt-commits: 910e4c850e0e43a1832f943e2782ffa8f06b3eb9
wpt-pr: 44300

UltraBlame original commit: b7468884afeb181ab7f2ec3224e73d56c0091360
marcoscaceres pushed a commit to web-platform-tests/wpt that referenced this pull request Feb 23, 2024
This CL implements the semantics specified in
https://wicg.github.io/observable/#dom-observable-foreach.

See WICG/observable#105.

For WPTs:
Co-authored-by: [email protected]

[email protected]

Bug: 1485981
Change-Id: I61344bad7fa4bac65146e1305a376fc1f5e55dc3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5249869
Reviewed-by: Mason Freed <[email protected]>
Commit-Queue: Dominic Farolino <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1257328}
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.

2 participants