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

Remove IteratorRemoteValue from the spec #691

Merged
merged 1 commit into from
Mar 21, 2024

Conversation

OrKoN
Copy link
Contributor

@OrKoN OrKoN commented Mar 20, 2024

Currently, how IteratorRemoteValue is produced is not specified as per #537

Additionally, a general iterator is any object with a next method as per https://tc39.es/ecma262/multipage/control-abstraction-objects.html#sec-iterator-interface and it is an interface that any object can implement.

Since there is currently no use case requiring serialization support of iterators, we propose removing IteratorRemoteValue from the spec.

The following use cases have been considered:

  1. Evaluating an iterator from the client code (In puppeteer code):
сonst it = await page.evaluateHandle(() => produce an iterator)
await it.evaluate(it => it.next())

Conclusion: serialization as an object is sufficient, since the user knows if they are dealing with an iterator and additionally they can check for the presence of the next method if needed.

  1. Logging an iterator object to console

We believe that serializing an iterator as an object would be sufficient.


Preview | Diff

Currently, how IteratorRemoteValue is not specified
as per #537

Additionally, a general iterator is any object with a
next method as per https://tc39.es/ecma262/multipage/control-abstraction-objects.html#sec-iterator-interface

Since there is currently no use case requiring serialzation support
of iterators, we propose removing IteratorRemoteValue from the spec.

The following use cases have been considered:

1) In puppeteer code,

```
сonst it = await page.evaluateHandle(() => produce an iterator)
await it.evaluate(it => it.next())
```

Conclusion: serialization as an object is sufficient, sicne the user
knows if they are dealing with an iterator and additionally they can
check for the presence of the next method if needed.

2) Logging an iterator object to console

We believe that serializing an iterator as an object, would be
sufficient.
@OrKoN OrKoN requested review from whimboo and jgraham March 20, 2024 10:33
@sadym-chromium
Copy link
Contributor

@jgraham @jimevans WDYT?

@OrKoN
Copy link
Contributor Author

OrKoN commented Mar 20, 2024

Does anyone have any use cases in mind that would require special handing of iterators during serialzation?

Copy link
Member

@jgraham jgraham left a comment

Choose a reason for hiding this comment

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

Since this is unused it's technically a noop change. But also I agree that since Iterator is just an interface we don't need a specific type.

@sadym-chromium sadym-chromium merged commit a6f087b into main Mar 21, 2024
5 checks passed
@sadym-chromium sadym-chromium deleted the orkon/remove-iterator-serialzation branch March 21, 2024 12:11
github-actions bot added a commit that referenced this pull request Mar 21, 2024
SHA: a6f087b
Reason: push, by sadym-chromium

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
sadym-chromium added a commit to GoogleChromeLabs/chromium-bidi that referenced this pull request Mar 21, 2024
After the `iterator` is [removed from the
spec](w3c/webdriver-bidi#691), there is no need
in this heuristics.

---------

Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: sadym-chromium <sadym-chromium@users.noreply.github.com>
@whimboo
Copy link
Contributor

whimboo commented Mar 25, 2024

@OrKoN can you please remove or better fix all the wpt tests for iterator serialization? Those are failing at the moment on wpt.fyi. Thanks.

@OrKoN
Copy link
Contributor Author

OrKoN commented Mar 25, 2024

@whimboo which tests do you mean? I have updated tests in web-platform-tests/wpt#45217 and they all pass for Firefox.

@whimboo
Copy link
Contributor

whimboo commented Mar 25, 2024

Oh wait. I was looking at eg this test. But yeah it passes now for Firefox and as it looks like requires a bidi mapper release for Chrome to pass the test?

@OrKoN
Copy link
Contributor Author

OrKoN commented Mar 25, 2024

Oh wait. I was looking at eg this test. But yeah it passes now for Firefox and as it looks like requires a bidi mapper release for Chrome to pass the test?

yes, I think chromium-bidi version is not the latest in wpt.fyi. Latest results can be viewed in https://googlechromelabs.github.io/chromium-bidi/.

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.

4 participants