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

Unserializable resources #35

Merged
merged 4 commits into from Oct 24, 2022
Merged

Conversation

danheuck
Copy link
Contributor

This PR allows for the creation of unserializable "client" resources as discussed in #34. The motivating example from that issue can now be rewritten using a resource:

use leptos::*;
use nokhwa::{js_camera::JSCameraConstraintsBuilder, JSCamera};
use std::{cell::RefCell, fmt::Debug, rc::Rc};

#[derive(Clone)]
struct Camera(Rc<RefCell<JSCamera>>);

impl Debug for Camera {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        f.debug_tuple("Camera")
            .field(&self.0.borrow().id())
            .finish()
    }
}

async fn get_camera(_: ()) -> Option<Camera> {
    let constraints = JSCameraConstraintsBuilder::new().build();
    let camera = JSCamera::new(constraints).await;
    camera.ok().map(|c| Camera(Rc::new(RefCell::new(c))))
}

#[component]
pub fn UserWebcam(cx: scope) -> Element {
    let camera = create_client_resource(cx, || (), get_camera);

    create_effect(cx, move |_| {
        if let Some(Some(camera)) = camera() {
            if let Err(e) = (*camera.0).borrow_mut().attach("camera_out", false) {
                log::error!("Failed to attach camera: {}", e);
            } else {
                log::info!("Attached camera");
            }
        }
    });

    view! {cx,
        <video id="camera_out" class="user_webcam"/>
    }
}

Sadly, I still couldn't get it to work properly with Suspense. There's probably a way to do it, but the camera has to use an effect to attach the media stream to the video HTML element and, if I wrap the video element in suspense, getting the timing to work properly such that the element was created before the effect ran didn't seem to work out for me.

I tested this with the existing examples that use resources (fetch and hackernews) which is why this PR also has an unrelated fix for an issue in hackernews-app's cargo.toml. The added code, particularly the new resource creation functions (create_client_resource() and create_client_resource_with_initial_value()), still needs documentation but I figured I'd throw the implementation up here for review.

@gbj
Copy link
Collaborator

gbj commented Oct 23, 2022

Yeah this looks really good — Existing automated tests still pass, the HackerNews demo works fine, and more than that I think it's a nice clean implementation.

If you want to really go above and beyond feel free to add some doctests. I still need to write docs for Resources in general so don't worry too much about it. Otherwise, I'm happy to merge this any time.

Here's one super-simple doctest just to make sure it compiles and runs with a type that isn't Serialize, Deserialize. I tried this and it seems fine. Feel free to use this as a template or expand.

/// # use leptos_reactive::*;
/// # create_scope(|cx| {
/// #[derive(Debug, Clone)] // doesn't implement Serialize, Deserialize
/// struct ComplicatedUnserializableStruct {
///   // something here that can't be serialized
/// }
/// // any old async function; maybe this is calling a REST API or something
/// async fn setup_complicated_struct() -> ComplicatedUnserializableStruct {
///   // do some work
///   ComplicatedUnserializableStruct { }
/// }
///
/// // create the resource that will
/// let result = create_client_resource(cx, move || (), |_| setup_complicated_struct());
/// # }).dispose();

@gbj
Copy link
Collaborator

gbj commented Oct 23, 2022

Oh my one other thought was on naming: Do you think it should actually be create_local_resource or something? It could just as well be a server-only resource as a client-only one! Open to your thoughts.

@danheuck
Copy link
Contributor Author

Updated documentation and changed "client" resources to "local" resources. Do you think it makes sense to replace all references to "unserializable" resources with "local" as well? I think it makes sense since you could have a resource that is technically serializable but you still want to run locally. My only issue is that I'm not sure what to call the "serializable" resources then. "Remote" would be the obvious counterpart, but in client-side rendering they'll still be run locally, so that name feels wrong. Maybe just "resource" and "local resource"?

@gbj
Copy link
Collaborator

gbj commented Oct 24, 2022

Nah I think it's fine to use "resource" and "local resource," and not worry about alternatives for serializable/unserializable... Those ones are pretty much descriptive, and not really part of the public API anyway.

Thanks so much for this contribution. I'm going to merge the PR. Great work!

@gbj gbj merged commit 88464fe into leptos-rs:main Oct 24, 2022
@danheuck danheuck mentioned this pull request Oct 24, 2022
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.

None yet

2 participants