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 iterable support for webidl #514

Closed
jonathanKingston opened this issue Jul 19, 2018 · 11 comments
Closed

Add iterable support for webidl #514

jonathanKingston opened this issue Jul 19, 2018 · 11 comments
Labels
frontend:webidl Issues related to the WebIDL frontend to wasm-bindgen help wanted We could use some help fixing this issue!

Comments

@jonathanKingston
Copy link
Contributor

In web-sys to get a functional DOM implementation iterable<Node?> is needed by NodeList.

https://heycam.github.io/webidl/#dfn-iterable-declaration which is supported by webidl::ast::Iterable.

pub fn get_elements_by_name(&self, element_name: &str) -> NodeList

@fitzgen fitzgen added help wanted We could use some help fixing this issue! frontend:webidl Issues related to the WebIDL frontend to wasm-bindgen labels Jul 19, 2018
@jonathanKingston
Copy link
Contributor Author

To add for_each support we will need to support #257

@fitzgen part of me things NodeList and others should just implement Iterator. I know this isn't the correct DOM API however the interface describes some of the same methods.
I don't know enough about the abstraction across the buffer but perhaps the nodes have already passed into wasm when code has called get_elements_by_name that it could be represented as a vec or similar in wasm?

The alternative is implement all the functions that iterables support in webidl like:
https://heycam.github.io/webidl/#es-forEach and https://heycam.github.io/webidl/#es-iterable-values

Oddly with the latest Option support I can't seem to provide a non throwing function return type. Is only Result<Option<T>, E> implemented for function responses and not: Option<T>?

I managed to get the code spitting out a values with a response, this should be implementing @@iterator perhaps it could just return a vector of the type?

Does this need a RFC?

@alexcrichton
Copy link
Contributor

Interesting! I think I'd personally be on board with the strategy of using Iterator in Rust and just defining it for iterable types where it makes sense. For example could get_elements_by_name return something like impl Iterator?

Returns from functions should support Option<T>, yeah, but do you have an example of a function that didn't work for you?

For now I think it's ok to avoid an RFC, we're still in the somewhat early stages of web-sys!

@Pauan
Copy link
Contributor

Pauan commented Jul 20, 2018

I don't think get_elements_by_name should return impl Iterator, because that would prevent using useful methods (like length, or indexing).

Instead I think the best course of action is to return NodeList (and then implement Iterator for NodeList).

@alexcrichton
Copy link
Contributor

Oh sure yes, if it's got an actual type (like NodeList does) then we should return that!

@jonathanKingston
Copy link
Contributor Author

jonathanKingston commented Jul 23, 2018

Returns from functions should support Option, yeah, but do you have an example of a function that didn't work for you?

let return_value = webidl::ast::ReturnType::NonVoid(self.value_type.clone());
let args = [];
first_pass
.create_basic_method(
    &args,
    Some(&"item".to_string()),
    &return_value,
    self_name,
    false,
    false
)
.map(wrap_import_function)
.map(|import| program.imports.push(import));

Where value_type is Node? from the webidl.
The above works when passing true to the throws argument.

I may put in a preliminary PR in with item and some of the other bits so then I can concentrate on the iterator here.

@alexcrichton
Copy link
Contributor

Ok sounds good to me, I can certainly help debug failures in a PR :)

@fitzgen
Copy link
Member

fitzgen commented Sep 12, 2018

see also #776

@derekdreery
Copy link
Contributor

I'm playing with window.fetch, the only way to inspect the contents of Response.headers is through the Symbol.iterator, either on the object itself or the contents of entries() (see MDN).

The iterable decl is documented in WebIDL level 1.

This iterable behavior isn't necessary for NodeList, because with that iterator the keys are consecutive numbers and there is a length property, which together can be used to manually iterate over it.

I'm going to have a dig through the codegen to see if its possible to generate js_sys::Iterator (or related) for iterables.

@derekdreery
Copy link
Contributor

derekdreery commented Nov 15, 2018

Ok so I've looked at the code and noticed that I seem to be starting from a blank slate. My implementation plan is

  1. Extract info from webidl
    • There should be exactly zero or one iterable declaration from any interface, partial interface, and any parent or child interfaces. Try to test for this as much as possible. There should also not be any operations with clashing names.
    • Collect it in an Option<IterableKind> where
      #[derive(Clone, Debug)]
      pub(crate) enum IterableKind<'src> {
          /// List-style iterable
          List {
              val_ty: &'src weedle::types::Type<'src>,
          },
          /// Map-style iterable
          Map {
              key_ty: &'src weedle::types::Type<'src>,
              val_ty: &'src weedle::types::Type<'src>,
         }
      }
      during the first pass
  2. Codegen
  • Don't do any codegen manually, just emit the four methods to the interface:

    In the ECMAScript language binding, an interface that is iterable will have “entries”, “forEach”, “keys”, “values” and @@iterator properties on its interface prototype object.

    So we just generate these 4 methods - the @@iterator behavior can be recovered from the entries
    method.

  • forEach takes a closure, but the other 3 methods need to use types from js_sys (Iterator). Do we
    do this for anything else in web-sys?

How do you feel about this approach generally?

@alexcrichton
Copy link
Contributor

That sounds like a good approach to me!

@daxpedda
Copy link
Collaborator

Fixed by #3962.
Though we decided to not encode the type in the iterator for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
frontend:webidl Issues related to the WebIDL frontend to wasm-bindgen help wanted We could use some help fixing this issue!
Projects
None yet
Development

No branches or pull requests

6 participants