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 more methods to UseDocument and UseWindow #39

Open
maccesch opened this issue Oct 21, 2023 · 11 comments
Open

Add more methods to UseDocument and UseWindow #39

maccesch opened this issue Oct 21, 2023 · 11 comments
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@maccesch
Copy link
Contributor

There are several methods which forward the calls, like UseDocument::body. Depending on the method if it already returns an Option<...> type it makes sense to add the postfix .unwrap_or_default() to return always none on the server.

@maccesch maccesch added enhancement New feature or request good first issue Good for newcomers labels Oct 21, 2023
@mamaicode
Copy link

Assign this to me please

@maccesch
Copy link
Contributor Author

Done. Thanks for taking this on!

@mamaicode
Copy link

Hi, I checked out the repo, correct me if I misunderstood.
UseDocument already has .unwrap_or_default()

@maccesch
Copy link
Contributor Author

maccesch commented Oct 23, 2023

Yes UseDocument is basically the same as Option<web_sys::Document>. It actually implements Deref for this type, so any method on Option is available on UseDocument.

But the idea is to implement methods that are available on web_sys::Document directly on UseDocument. Like already done with body for example.

As you can see there's actually a macro to help with this. It is just a matter of going through the methods on web_sys::Document and seeing if it makes sense to have the same return type (probably the case for most if not all methods) or wrap the return type in Option.

@maccesch
Copy link
Contributor Author

maccesch commented Jan 2, 2024

@mamaicode Any updates on this?

@luckynumberke7in
Copy link
Contributor

Hey, thinking about tackling this!

I just want to clarify my understanding from reading your request.

So for this, I'd go into use_document.rs and create a ton of impl's something like:

impl_ssr_safe_method!(
        set_body(&self, value: Option<&web_sys::HtmlElement>) ;
    );

referencing all implementations from the page: https://rustwasm.github.io/wasm-bindgen/api/web_sys/struct.Document.html#

Does this look about right?

@maccesch
Copy link
Contributor Author

maccesch commented Mar 6, 2024

That's great, thanks!

Yes exactly. You don't have to implement every last one. I can imagine there might be some that don't make sense, but you can be the judge of that. And then it makes sense to see what to do about the return type. If it already returns an Option<T> we can flatten the output because an Option<Option<T>> doesn't make much sense (see for example the body method).

Similar in the case of a Result<Option<T>, Err> (see method query_selector).

Let me know if there are any more questions.

@luckynumberke7in
Copy link
Contributor

luckynumberke7in commented Mar 8, 2024

Thanks for the additional input, I'm taking this on specifically to master the type conversions in Rust/Leptos.

Honestly...I may do this in a few steps, since there are so many. I can think of several to start with for personal convenience, but do you have any tips on importance for this project specifically? @maccesch

@luckynumberke7in
Copy link
Contributor

I've started looking at the 4 completed examples for inspiration to start and I noticed 3 of them look basically the same, however query_selector_all from web_sys has the signature query_selector_all(&self, selectors: &str) -> Result<NodeList, JsValue>

In this instance you added an Option and didn't do any unwrapping, can you elaborate on the reasoning behind this a little more for me? @maccesch

Thanks.

@luckynumberke7in
Copy link
Contributor

Ok, sorry if I'm blowing you up! 1 more question so far, I've run into a few that have no return type, such as pub fn set_title(&self, value: &str); do we need to do anything about this, or was this one of the ones that don't make sense to implement?

@maccesch
Copy link
Contributor Author

maccesch commented Mar 9, 2024

Hey thanks for taking a good look at this!

There's no priorities at all and you can totally feel free to do it in however many steps are convenient for you.

Re query_selector_all: These functions need to work with SSR. That's the whole point, right? So one would think that returning Ok(...) with an empty NodeList would be a good default on the server but unfortunately you can't instantiate web_sys types on the server. That's why I didn't unwrap and let the Option stay there.

Re set_title: I wouldn't say that this is useless. You might have to modify the macro to account for functions without return type or write a new one for that though. I'm not sure.

I hope this helps!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants