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

Add webidl iterable support #3962

Merged
merged 3 commits into from
Jul 28, 2024

Conversation

RaoulHC
Copy link
Contributor

@RaoulHC RaoulHC commented May 15, 2024

Was trying to figure out how to list files in OPFS before I realised these methods were just missing from web-sys.

Added some tests that use these and more generally test some of the file system API, only tested it with geckodriver but it should be well supported across all browsers.

Also added some flags so that the optional stream module should get generated in docs.rs too, wasn't obvious it existed, and it's somewhat needed to make use of the AsyncIterator that these methods return.

@RaoulHC
Copy link
Contributor Author

RaoulHC commented May 16, 2024

Oh I see I missed that the web-sys files are generated from webidl, and that the async iterable field is what should be providing the entries/keys/values methods. Given the following code am I right in thinking this isn't currently supported?

InterfaceMember::AsyncIterable(_iterable) => {
log::warn!(
"Unsupported WebIDL async iterable interface member: {:?}",
self
);
Ok(())
}

@Liamolucko
Copy link
Collaborator

Given the following code am I right in thinking this isn't currently supported?

InterfaceMember::AsyncIterable(_iterable) => {
log::warn!(
"Unsupported WebIDL async iterable interface member: {:?}",
self
);
Ok(())
}

Yes; there's an old issue #514 about it.

@RaoulHC
Copy link
Contributor Author

RaoulHC commented May 16, 2024

Yes; there's an old issue #514 about it.

Yep found that and am getting something working, looks like it's relatively simple to add, most of the plumbing is already there, so will update this PR.

@RaoulHC RaoulHC force-pushed the raoul/missing-directory-methods branch 2 times, most recently from 6cc1be2 to 6047da3 Compare May 16, 2024 08:58
@RaoulHC RaoulHC changed the title Add missing FileSystemDirectoryHandle methods Add webidl iterable support May 16, 2024
@RaoulHC
Copy link
Contributor Author

RaoulHC commented May 16, 2024

I've got something working that I've pushed, wasn't sure how to do the async forEach method so I've left that for now, I think most users will probably want to use the futures Stream interface via JsStream anyway.

Looking at how the ergonomics could be improved given we know the types yielded for these cases, some ideas in that issue so will see if something can work.

@RaoulHC
Copy link
Contributor Author

RaoulHC commented May 17, 2024

Playing around with it a bit, I think the neatest way might be leave the exposed js methods (maybe prepend with a js_?) but also generate a shim that uses the type information to give a generate an (async) iterator that uses unchecked_into to massage it into something nicer to deal with in rust.

So for FileSystemDirectoryHandle it'd be something like this:

impl FileSystemDirectoryHandle
   pub fn entries() -> impl Stream<Item = (JsString, FileSystemHandle)> {
       JsStream::from(self.js_entries()).map(|x| {
            let array = x.unwrap().unchecked_into::<Array>();
            (array.at(0).unchecked_into(), array.at(1).unchecked_into())
        })
    }
}

with something similar for regular iterables, and without the array step for keys and values, which only return the one item.

Thoughts? It seems like there was a bunch of discussions regarding iterators but the direction was never really settled and a lot of those main contributors are no longer on wasm-bindgen, Probably need to read a bit more into if there are ever any cases where iterables for some of these methods can throw errors, but hand writing this for the directory case works fairly nicely with the tests I have.

@RaoulHC
Copy link
Contributor Author

RaoulHC commented May 21, 2024

I've had a bit of a crack at this, but how you massage the types was trickier than I thought, Web IDL has types that JS doesn't so to cast to equivalent rust types you have to go via js types which doesn't always cast simply. Some API's generate enums that I also wasn't sure exactly how to deal with (see MediaKeyStatus in MediaKeyStatusMap. There's also the question of whether we want to stick to Js values (i.e. JsString and js_sys::Number) or convert to rust values closer to what Web IDL specifies: sticking to js values means there wont be unnecessary copies, but you lose some of the type information specified for the API.

I think for now exposing these methods that just return js iterators allows users to at least use the iterable interfaces, they'll just have to deal with casting to the types they want. Maybe we want to gate it behind a feature flag if we plan on changing the API later.

Copy link
Collaborator

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

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

LGTM!
Just missing a changelog entry.

I know that the current state isn't ideal with the fact that we don't encode the types that we are already able to parse from the WebIDL, but this will require much more design work and probably best to leave to a future breaking change.

Would appreciate your approval on this as well @Liamolucko.

crates/futures/src/lib.rs Show resolved Hide resolved
#[doc = "[MDN Documentation](https://developer.mozilla.org/en-US/docs/Web/API/DOMTokenList/forEach)"]
#[doc = ""]
#[doc = "*This API requires the following crate features to be activated: `DomTokenList`*"]
pub fn for_each(this: &DomTokenList, callback: &::js_sys::Function) -> Result<(), JsValue>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Array::for_each() takes a &mut dyn FnMut() instead, which also can encode types. Both ways have tradeoffs and ideally we could have both, so I'm happy to leave it as is because I think it aligns more with all the other APIs web-sys currently exposes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah while I think we eventually want that, I think it will need shims that aren't so easy to generated as I struggled with for the entries and co API too. Can write a new issue for doing this specifically (although maybe it fits one of the existing related issues already, and that just needs an update).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably be best to wait for demand first.
But I appreciate the offer!

@daxpedda daxpedda requested a review from Liamolucko June 23, 2024 19:15
@daxpedda daxpedda added the waiting for author Waiting for author to respond label Jun 23, 2024
@RaoulHC RaoulHC force-pushed the raoul/missing-directory-methods branch 2 times, most recently from 159dcda to 237d3bf Compare July 12, 2024 15:09
RaoulHC added a commit to RaoulHC/wasm-bindgen that referenced this pull request Jul 12, 2024
@RaoulHC
Copy link
Contributor Author

RaoulHC commented Jul 12, 2024

Rebased, added a changelog entry and updated the CI so I think this should be good to go in now.

RaoulHC added a commit to RaoulHC/wasm-bindgen that referenced this pull request Jul 12, 2024
@RaoulHC RaoulHC force-pushed the raoul/missing-directory-methods branch from 237d3bf to 99de505 Compare July 12, 2024 15:21
@RaoulHC
Copy link
Contributor Author

RaoulHC commented Jul 12, 2024

Not sure what the typescript test failure is, but I seem to be getting it on main too, so it's unrelated to my change.

Copy link
Collaborator

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

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

Rebased, added a changelog entry and updated the CI so I think this should be good to go in now.

Apologies for the delay!

Not sure what the typescript test failure is, but I seem to be getting it on main too, so it's unrelated to my change.

That was fixed in #4016.

CHANGELOG.md Outdated Show resolved Hide resolved
#[doc = "[MDN Documentation](https://developer.mozilla.org/en-US/docs/Web/API/DOMTokenList/forEach)"]
#[doc = ""]
#[doc = "*This API requires the following crate features to be activated: `DomTokenList`*"]
pub fn for_each(this: &DomTokenList, callback: &::js_sys::Function) -> Result<(), JsValue>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably be best to wait for demand first.
But I appreciate the offer!

RaoulHC added 3 commits July 28, 2024 21:24
Use the doc_cfg feature to show it's behind futures-core-03-stream
feature flag.
Most of the plumbing was there but now (async) iterable instances
generate the `entries`, `keys` and `values` methods. `forEach` is also
generated for the non-async iterable.
@daxpedda daxpedda force-pushed the raoul/missing-directory-methods branch from 99de505 to 41f2bfc Compare July 28, 2024 19:25
@daxpedda daxpedda removed the waiting for author Waiting for author to respond label Jul 28, 2024
@daxpedda daxpedda merged commit 240fbd2 into rustwasm:main Jul 28, 2024
25 checks passed
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.

3 participants