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

Resource::set() breaks Suspense if it tracks multiple resources. #1905

Closed
luxalpa opened this issue Oct 17, 2023 · 1 comment · Fixed by #1985
Closed

Resource::set() breaks Suspense if it tracks multiple resources. #1905

luxalpa opened this issue Oct 17, 2023 · 1 comment · Fixed by #1985
Labels
bug Something isn't working
Milestone

Comments

@luxalpa
Copy link
Contributor

luxalpa commented Oct 17, 2023

Describe the bug
When tracking more than one resource within the body of a component, then updating the resources using the Resource::set function gives incorrect values for SuspenseContext.pending_resources, which causes the Suspense to stay forever in pending state.

Leptos Dependencies
currently using rev 17b3300 (according to lockfile)

[patch.crates-io]
leptos_reactive = { git = "https://github.com/leptos-rs/leptos.git", branch = "main" }
leptos = { git = "https://github.com/leptos-rs/leptos.git", branch = "main" }
leptos_meta = { git = "https://github.com/leptos-rs/leptos.git", branch = "main" }
leptos_actix = { git = "https://github.com/leptos-rs/leptos.git", branch = "main" }
leptos_router = { git = "https://github.com/leptos-rs/leptos.git", branch = "main" }
leptos_hot_reload = { git = "https://github.com/leptos-rs/leptos.git", branch = "main" }
leptos_macro = { git = "https://github.com/leptos-rs/leptos.git", branch = "main" }
leptos_integration_utils = { git = "https://github.com/leptos-rs/leptos.git", branch = "main" }

To Reproduce

#[component]
pub fn MyTest() -> impl IntoView {
    let r1 = create_local_resource(move || {}, move |_| future::pending::<()>());
    let r2 = create_local_resource(move || {}, move |_| future::pending::<()>());

    let click = move |_| {
        r1.set(());
        r2.set(());
        logging::log!("Setting resources");
    };

    view! {
        <button on:click=click>"Click"</button>
        <Suspense fallback=move || view! { <p>"Loading..."</p> }>
            {move || {
                r1.track();
                r2.track();
                view! {
                    <div>"My Test"</div>
                }
            }}
        </Suspense>
    }
}

Clicking the button does not cause the Suspense to resolve. In fact, when monitoring the Suspense context from within the suspense, I notice that the number of pending_resources goes down to 1 and then stays there.

Expected behavior
Expected the Suspense to move into its resolved state once both resources have been completed.

Additional context

  • The issue does not arise at the time the Resource completes automatically (i.e. with a gloo_timers::future::sleep(Duration::from_secs(10)).await; instead of the ever-pending future). However, even with such a future, calling set on the Resource still causes the same errorneous behaviour.
  • When only calling set on a single resource (either r1 or r2 but not both), calling set() multiple times on the same resource actually causes the pending_resources value to increase (despite no additional resources being added). The first set keeps the value the same, but subsequent calls increase the value. Running refetch on the resource slightly alters the behavior as it will cause even the first set to already increment the value.

The use-case is for a cache similar to leptos_query, where resources can be set when data arrives in the cache from various sources.

@gbj gbj added the bug Something isn't working label Oct 17, 2023
@gbj
Copy link
Collaborator

gbj commented Oct 19, 2023

Specifically in this case, setting two resources synchronously but without batching breaks a Suspense that tracks them both. You'll notice that in this example, switching the click handler to the following does work:

batch(move || {
    r1.set(());
    r2.set(());
});

This is because the first set() triggers the move || block inside the Suspense, which tracks both of the resources, and then it re-increments the suspense counter for the second one, leaving it hanging at 1.

If the two resources are tracked in a fine-grained way, such that triggering one doesn't trigger something that reads from the other one again, that also works

<Suspense fallback=move || view! { <p>"Loading..."</p> }>
    <div>
        {move || format!("{:?}", r1.get())}
        {move || format!("{:?}", r2.get())}
    </div>
</Suspense>

In the same way, tracking them non-reactively also works.

<Suspense fallback=move || view! { <p>"Loading..."</p> }>
    {r1.track(); r2.track();}
    <div>
        "Hello!"
    </div>
</Suspense>

This is obviously bugged — the interaction between resources and Suspense doesn't work well given the ability to .update() and .set() them.

It's possible that the Suspense context should really hold something like a HashSet<ResourceId> rather than a usize to correctly track the total number of pending resources. This is a little more expensive and way overkill for the simplest cases, but seems like it's much more correct in these more complex cases.

@gbj gbj added this to the 0.6 milestone Nov 3, 2023
gbj added a commit that referenced this issue Nov 4, 2023
…closes `Suspense` only working with a single `Resource` (closes #1805, closes #1905)
@gbj gbj closed this as completed in #1985 Nov 4, 2023
gbj added a commit that referenced this issue Nov 4, 2023
…closes `Suspense` only working with a single `Resource` (closes #1805, closes #1905) (#1985)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants