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

Initial support for Document, EventTarget, NodeList and Iterator #541

Merged

Conversation

jonathanKingston
Copy link
Contributor

@alexcrichton https://github.com/rustwasm/wasm-bindgen/compare/master...jonathanKingston:web-sys/document-support?expand=1#diff-9f9955b41f2425125eaf42c53a05c94dR558 is the mentioned issue from #514 (comment)

The other question I had was surrounding: element.query_selector_all('x').get(0) returning a Node not an Element. Is this something rustwasm/rfcs#2 will tackle?

@alexcrichton
Copy link
Contributor

Awesome, thanks for this!

I've switched the throws there to false and I think I see the issue! The first problem is that Option wasn't defined in this list. After that though it looked like the path traversal code wasn't handling generics so there were lots of compilations errors. After filling out the missing cases though it should be all good to go!

first_pass
.create_basic_method(
&args,
Some(&"values".to_string()),
Copy link
Contributor

Choose a reason for hiding this comment

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

I wasn't quite sure what these two methods were doing, is this what it means to be iterable? (a values and an item method?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It comes from the iterable in webidl yeah: https://heycam.github.io/webidl/#es-iterable

I couldn't get forEach to work as we don't have callbacks yet.

entries, keys and values and some other behaviour is needed.

I messed up though item is a getter on the webidl which isn't supported yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for this direction, it really helped me understand the codebase a lot more too!

@jonathanKingston
Copy link
Contributor Author

I filled in the Parenthesized PathArguments and it looks like it is working (I have more in Element now like getAttribute which wasn't working before).

I commented out the code for now as I think I need to implement a trait for iterator similar to how OptionFromWasmAbi and OptionIntoWasmAbi work.

I think perhaps I could remove the function comments and it should be merged as is (without iterator).

for input in data.inputs.iter() {
input.imported_types(f);
}
// TODO do we need to handle output here?
Copy link
Contributor

Choose a reason for hiding this comment

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

I think so yeah if you get a chance, makes sense to go ahead and add it here!

@alexcrichton alexcrichton merged commit 4b4bed5 into rustwasm:master Jul 24, 2018
@alexcrichton
Copy link
Contributor

Awesome looks great to me!

Do you think we'll need a new trait for the iterator types? I'm actually curious if we can get by with an Iterator trait implementation? Or perhaps things like values return impl Iterator?

@jonathanKingston
Copy link
Contributor Author

Wow cool thanks for merging!

Do you think we'll need a new trait for the iterator types? I'm actually curious if we can get by with an Iterator trait implementation? Or perhaps things like values return impl Iterator?

More experimenting needed I think, I am hoping we can use that but I'm still learning here.

@alexcrichton
Copy link
Contributor

Sure thing! I'm all for incremental changes anyway :)

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