Suspense Support #2212

futursolo · 2021-11-28T10:05:00Z

Description

This pull request introduces an experimental Suspense support.

Summary:

Added HtmlResult as an alias to RenderResult<Html>.
A RenderError is added with Suspended being the only variant at the moment.
A <Suspense /> component is added that renders a fallback UI when receiving a Suspension.
A Suspension and SuspensionHandle type to suspend component rendering.
A VSuspense variant on virtual dom to facilitate special rendering behaviour (preserves component state when suspended).

Checklist

I have run cargo make pr-flow
I have reviewed my own code
I have added tests

futursolo · 2021-11-28T10:07:35Z

I imagine that this needs a lot of tests. So I submitted the PR as I add tests so anyone who wants to give it a shot can have a try and see if the design have flaws / can be improved.

Mainly now Html is a result, so it will break things. But none of the examples broke badly and are quick to fix.

github-actions · 2021-11-28T10:09:52Z

Visit the preview URL for this PR (updated for commit 5a376af):

https://yew-rs--pr2212-suspense-7k6vaamf.web.app

_{(expires Wed, 12 Jan 2022 11:05:27 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

WorldSEnder · 2021-11-29T11:35:43Z

If there's a VSuspense variant on VNode, why is Err(RenderError::Suspended) even needed? As far as I see, this is never even instantiated, only matched against.

futursolo · 2021-11-29T12:50:47Z

If there's a VSuspense variant on VNode, why is Err(RenderError::Suspended) even needed? As far as I see, this is never even instantiated, only matched against.

These 2 are used for different purposes.

VSuspense is used by <Suspense /> component to switch between fallback and children where as RenderError::Suspended is returned by the component that actually suspends.

RenderError::Suspended is almost never constructed but includes a from implementation of Suspension.

A hook that suspends a component returns a Err(Suspension) when it wants to suspend rendering of the component and it will be converted to Err(RenderError::Suspended) by ?.

futursolo · 2021-11-30T14:19:20Z

I have added tests and docs to this PR and now it's ready for review.

voidpumpkin · 2021-11-30T16:46:01Z

Sorry this PR will probably be a hostage until we actually release 0.19

ranile · 2021-12-14T14:41:59Z

I really don't like this change

Html is now aliased to RenderResult<VNode>.

This means that every view method/function component now needs to return an Ok(_). The html! macro should not return a Result since the generated code isn't fallible. We need to look into other ways of implementing this functionality before merging this in.

As a side note, can we not call it "Suspense"? It seems like everything in Yew is ripped off from React and even in React, Suspense name isn't a clear representation of what it does.

futursolo · 2021-12-15T11:26:38Z

I really don't like this change

Html is now aliased to RenderResult<VNode>.

This means that every view method/function component now needs to return an Ok(_). The html! macro should not return a Result since the generated code isn't fallible. We need to look into other ways of implementing this functionality before merging this in.

I also wish there's a better way to do this. However, Suspense requires an early interruption of the function component rendering flow. This is done via throw in JavaScript. In Rust, there're 2 ways that this could be done:

Result with ?
Macros

There's no fundamental difference between these 2 approaches as ? is simply a macro (used to be try! macro) that expands to Err(e) => return Err(e). For the macro-based approach, all hooks that suspends needs to be a macro (e.g.:use_query!(), which internally try!'s automatically).

As for type Html = RenderResult<VNode>; & html!, there're a couple ways to implement this:

A:

type Html = RenderResult<VNode>;

#[function_component(Comp)]
fn comp() -> Html {
    html! { ... }
}

B:

type HtmlResult = RenderResult<Html>;

#[function_component(Comp)]
fn comp() -> HtmlResult {
    Ok(html! { ... })
}

C:

// Magic in Procedural Macro, Suspends with custom syntax.
#[function_component(Comp)]
fn comp() -> Html {
    html! { ... }
}

After evaluating the design, I think B's design is the most "correct" design but likely to cause difficulty in migration.
As for C, I cannot come up with something better than ? as the suspending operator.

Hence, I have picked A.

As a bonus point, I was able to fixed all examples with very little changes (in ~5 mins).

As a side note, can we not call it "Suspense"? It seems like everything in Yew is ripped off from React and even in React, Suspense name isn't a clear representation of what it does.

I think suspense naming is appropriate here.

<Suspense /> naming is also used by Vue (and Preact) and already widely accepted by React & Vue developers.

One point that should be taken into consideration is that:
Suspense is not a single feature, it's the core of an entire set of features
(SuspenseList, useTransition, useDeferredValue, and more).
I plan to gradually implementing those for Yew as well if this PR is accepted in some form.
If we rename Suspense, should we do the same to other features as well?

In addition, none of these features are straightforward to understand.
If we don't use the same name as React, we are likely going to have a note in the doc:
"Yew's <LoadingPlaceholder /> functions like React's <Suspense />".

Which kind of achieves the same effect of saying that this is "inspired" by React.

ranile · 2021-12-16T09:57:19Z

Instead of returning Result<VNode>, we could do the following:

struct VSuspense {
    fallback: VNode,
    content: VNode,
}

enum VNode {
    // ...
    VSuspense(VSuspense)
}

// in Suspense component

#[derive(Properties, PartialEq)]
struct SusProps {
    fallback: Html,
    children: Html,
}

#[function_component(Suspense)]
fn sus(props: &SusProps) -> Html {
    VNode::VSuspense(VSuspense {
        fallback: props.fallback.clone(),
        content: props.children.clone()
    })
}


// somewhere else

let fallback = html! { <Fallback /> };

html! {
     <Suspense {fallback}>
         <div>content</div>
     </Suspense>

We could also implement the Try trait so (on nightly) ? can also be used.

futursolo · 2021-12-16T11:47:40Z

Instead of returning Result<VNode>, we could do the following:

struct VSuspense {
    fallback: VNode,
    content: VNode,
}

enum VNode {
    // ...
    VSuspense(VSuspense)
}

// in Suspense component

#[derive(Properties, PartialEq)]
struct SusProps {
    fallback: Html,
    children: Html,
}

#[function_component(Suspense)]
fn sus(props: &SusProps) -> Html {
    VNode::VSuspense(VSuspense {
        fallback: props.fallback.clone(),
        content: props.children.clone()
    })
}


// somewhere else

let fallback = html! { <Fallback /> };

html! {
     <Suspense {fallback}>
         <div>content</div>
     </Suspense>

We could also implement the Try trait so (on nightly) ? can also be used.

The current implementation already has a VSuspense variant on VNode. And the <Suspense /> component is not the reason why we need to make Html = RenderResult<VNode>.

See: https://github.com/yewstack/yew/pull/2212/files#diff-0cd3c02c95dd9d4c5a156d6ac41322a90b238aa47a686cb052e35da7f13c7112R79

The place that actually needs Err(RenderError::Suspended(_)) is the component that is currently suspended.

See: https://github.com/yewstack/yew/pull/2212/files#diff-69584225471214af964defca7edc8911d478edfc3608e8f38faf17d17b23121bR17

Please see my previous comment about why VSuspense is not a replacement for RenderResult<VNode>: #2212 (comment)

voidpumpkin · 2021-12-17T14:57:07Z

I have mixed views on this feature. And I am writing this while i looked only to the modified examples code, so pardon me if some comments are not well-informed.

Pros

It will let us write less boilerplate in prod code that is like this:

let bob = match fetched_bob {
    Some(some) => some,
    None => return html!{<MyFallback />},
}

Though to be honest the upper thing could be solved simply by getting let else statement stable https://rust-lang.github.io/rfcs/3137-let-else.html

Other than that I don't see the benefit of specifying the fallback on the parent.

Though can the suspense cascade up the tree more than once?
If the answer is yes i think that's a big pro as well, to have one fallback for all the child component tree.

Cons?

I ended up rambling in cons so you can tldr it at the end
The dark side of this change is that people might end up implementing custom into conversions from all errors to this unified error that gets returned in the view function.

While inherently its not a bad thing, and definitely it will lessen the amount of boilerplate error handling, I think it a little drives away from the rust model where all errors should be explicitly handled.
It almost feels like suddenly we have throw functionality in yew.

But then again it kind of still makes sense because its like collecting all the errors at the root DOM done and not rendering anything faulty.

TLDR: I see only pros with the feature idea. I don't see any cons that actually forces anyone into a bad scenario.

voidpumpkin · 2021-12-17T15:01:49Z

Regarding the A, B and C options.
I would also go for B as it properly states we are returning a Result there.

voidpumpkin

I went through the PR, now i realize i was mixing up Suspense with error boundaries a little, sorry for that.
I am rejecting until proper steps are made towards A, B or C options, depending on which one we decide to use in this PR discussion.

# Conflicts: # packages/yew-macro/src/function_component.rs

futursolo · 2021-12-20T14:35:54Z

After some consideration, I have decided not to choose Option D.

For the following reasons:

I have chosen not to include struct component because it's likely that people are going to write code that are less than ideal and it's hard to explain why you shouldn't do that. (See: Suspense Support #2212 (comment)) Limiting suspension to hooks mitigates this issue greatly as you can only have a fixed number of hooks in a component.
One of the primary reasons of type Html = RenderResult<VNode> is that it would require a rewrite of every component for no good reason and having to do type Render = Html for every component sort of achieves the same effect (annoyance?).
The change set is unnecessarily big for one feature (requires a rewrite of every component, which is also the original reason why Option B is not selected).

ranile

There's files committed (shown in the diff) but without any changes. Can you fix that?

ranile · 2021-12-20T14:59:42Z

packages/yew-macro/src/function_component.rs

+        // When a component ends with Internal, it will become possessive.
+        // InternalsInternal instead of InternalInternal
+        provider_name_str.push_str("sInternal");


This doesn't cover the following case: InternalsInternal. That will become InternalssInternal. Besides, why even bother with the provider name? It's not like it really matters.

InternalsInternal's Internal will become InternalsInternalsInternal.

examples/suspense/Cargo.toml

examples/suspense/src/use_sleep.rs

ranile · 2021-12-20T15:05:26Z

packages/yew-macro/tests/function_component_attr/generic-pass.rs

+// no longer possible?
+// #[::yew::function_component(ConstGenerics)]
+// fn const_generics<const N: ::std::primitive::i32>() -> ::yew::Html {
+//     ::yew::html! {
+//         <div>
+//             { N }
+//         </div>
+//     }
+// }


What happened here? Seems like this PR broke something since the CI passes on master.

const generics are not possible in functions and the updated implementation now uses a function.

packages/yew/src/html/component/mod.rs

packages/yew/src/suspense/suspension.rs

ranile · 2021-12-20T15:21:22Z

website/docs/concepts/suspense.md

+hook should return a `Err(Suspension)` and users should unwrap it with
+`?` in which it will be converted into `Html`.
+
+```rust ,ignore


Can we not ignore this?

Not ignoring it would require including a complete hook implementation on every snippet.

I have included a complete example below.

I wonder how long will it take for docusaurus to finally merge this in facebook/docusaurus#5783
Then we could include all needed code and just hide it.

ranile · 2021-12-20T15:22:26Z

website/docs/concepts/suspense.md

+can use a function component as a Higher-Order-Component to
+achieve suspense-based data fetching.
+
+```rust, ignore


Again, let's not ignore here the snippets

voidpumpkin

Well looks fine to me, though i fully admit that due to me not contributing that much code wise i can miss things.

# Conflicts: # packages/yew/src/html/component/scope.rs # packages/yew/src/lib.rs # website/docs/concepts/function-components/custom-hooks.mdx # website/docs/concepts/function-components/pre-defined-hooks.mdx

voidpumpkin · 2021-12-30T11:35:03Z

I wonder if suspense is a good feature to have considering you could reproduce this error very easily as using a ? essentially is an early return

(but i need to test this)

futursolo · 2021-12-30T11:38:41Z

I wonder if suspense is a good feature to have considering you could reproduce this error very easily as using a ? essentially is an early return

(but i need to test this)

There's a test covering early return, and it's not reproducible in the test case.

https://github.com/yewstack/yew/pull/2212/files#diff-a17faa4af94b27d70b057bc6bb4a8b12df9e0eb35597a0c5ac9c9fa26bc689f1R203

I think the issue you are experiencing is something different. Because the number of hooks is not limited at the moment.
If it is limited after first call, you should get a different error (https://github.com/yewstack/yew/blob/master/packages/yew/src/functional/hooks/mod.rs#L58).

The error mentioned in the discord (where hooks fail to downcast): https://github.com/yewstack/yew/blob/master/packages/yew/src/functional/hooks/mod.rs#L72

# Conflicts: # packages/yew/tests/use_state.rs

ranile

LGTM

I will admit, I didn't inspect literally everything but it this should be good. If something comes up, it can be fixed later on.

WorldSEnder · 2022-01-05T15:28:29Z

packages/yew/src/virtual_dom/mod.rs

+    /// Move elements from one parent to another parent.
+    /// This is currently only used by `VSuspense` to preserve component state without detaching
+    /// (which destroys component state).
+    /// Prefer `detach` then apply if possible.
+    fn shift(&self, previous_parent: &Element, next_parent: &Element, next_sibling: NodeRef);
+


Can you comment why this can't reuse VNode::move_before? There is a slight difference in that the next_sibling is a NodeRef instead of the dereferenced Option<&Node> but I don't immediately see how that matters.

move_before assumes previous position and next position are under the same parent and hence does not attempt to update ComponentState where as shift has a different parent and hence is updated via push_component_update for VComp.

* Make Html a Result. * Fix tests. * Implement Suspense. * Schedule render when suspension is resumed. * Shift children into a detached node. * styled example. * Update wording a little bit. * Move hint to hint. * Add some tests. * Fix clippy. * Add docs. * Add to sidebar. * Fix syntax highlight. * Component -> BaseComponent. * Html -> VNode, HtmlResult = RenderResult<Html>. * Suspendible Function Component. * Add a method to create suspension from futures. * Revert extra changes. * Fix tests. * Update documentation. * Switch to custom trait to make test reliable. * Fix file permission. * Fix docs. * Remove log. * Fix file permission. * Fix component name error. * Make Suspension a future.

futursolo added 9 commits November 27, 2021 02:47

Make Html a Result.

fba4a5d

Fix tests.

4b6c311

Implement Suspense.

0f5f307

Schedule render when suspension is resumed.

7f19683

Shift children into a detached node.

31332c6

Merge branch 'master' into suspense

0c4a59b

styled example.

9bef4de

Update wording a little bit.

aa766b7

Move hint to hint.

1fc0db9

futursolo added 6 commits November 30, 2021 21:59

Add some tests.

376413a

Merge branch 'master' into suspense

b11e503

Fix clippy.

2e7f0bf

Add docs.

77ab06a

Add to sidebar.

9afeef0

Fix syntax highlight.

4d58a14

voidpumpkin added A-yew Area: The main yew crate A-yew-macro Area: The yew-macro crate labels Dec 15, 2021

voidpumpkin requested changes Dec 17, 2021

View reviewed changes

futursolo added 5 commits December 20, 2021 21:22

Add a method to create suspension from futures.

af4e2f5

Merge branch 'master' into suspense

06a1954

# Conflicts: # packages/yew-macro/src/function_component.rs

Revert extra changes.

cbf12be

Fix tests.

b664e5f

Update documentation.

066662d

Switch to custom trait to make test reliable.

f097303

ranile requested changes Dec 20, 2021

View reviewed changes

futursolo added 6 commits December 21, 2021 18:50

Fix file permission.

d8e9701

Fix docs.

787de4f

Remove log.

d8c3e85

Fix file permission.

7e66418

Resolve Conflict.

054f7fa

Fix component name error.

eb950d2

futursolo requested review from voidpumpkin and ranile December 24, 2021 13:02

voidpumpkin previously approved these changes Dec 26, 2021

View reviewed changes

futursolo added 2 commits December 30, 2021 19:47

Merge branch 'master' into suspense

23c9a95

# Conflicts: # packages/yew/src/html/component/scope.rs # packages/yew/src/lib.rs # website/docs/concepts/function-components/custom-hooks.mdx # website/docs/concepts/function-components/pre-defined-hooks.mdx

Make Suspension a future.

fae94d1

futursolo dismissed voidpumpkin’s stale review via fae94d1 December 30, 2021 11:16

Merge branch 'master' into suspense

fcc8b68

# Conflicts: # packages/yew/tests/use_state.rs

voidpumpkin approved these changes Jan 3, 2022

View reviewed changes

Merge branch 'yewstack:master' into suspense

5a376af

ranile approved these changes Jan 5, 2022

View reviewed changes

ranile merged commit ac3af0a into yewstack:master Jan 5, 2022

WorldSEnder reviewed Jan 5, 2022

View reviewed changes

futursolo deleted the suspense branch January 27, 2022 13:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Suspense Support #2212

Suspense Support #2212

futursolo commented Nov 28, 2021 •

edited

Loading

futursolo commented Nov 28, 2021 •

edited

Loading

github-actions bot commented Nov 28, 2021 •

edited

Loading

WorldSEnder commented Nov 29, 2021

futursolo commented Nov 29, 2021

futursolo commented Nov 30, 2021 •

edited

Loading

voidpumpkin commented Nov 30, 2021

ranile commented Dec 14, 2021

futursolo commented Dec 15, 2021

ranile commented Dec 16, 2021

futursolo commented Dec 16, 2021

voidpumpkin commented Dec 17, 2021 •

edited

Loading

voidpumpkin commented Dec 17, 2021

voidpumpkin left a comment

futursolo commented Dec 20, 2021

ranile left a comment

ranile Dec 20, 2021

futursolo Dec 20, 2021

ranile Dec 20, 2021

futursolo Dec 20, 2021 •

edited

Loading

ranile Dec 20, 2021

futursolo Dec 21, 2021

voidpumpkin Dec 26, 2021

ranile Dec 20, 2021

voidpumpkin left a comment

voidpumpkin commented Dec 30, 2021 •

edited

Loading

futursolo commented Dec 30, 2021 •

edited

Loading

ranile left a comment

WorldSEnder Jan 5, 2022

futursolo Jan 5, 2022

Suspense Support #2212

Suspense Support #2212

Conversation

futursolo commented Nov 28, 2021 • edited Loading

Description

Checklist

futursolo commented Nov 28, 2021 • edited Loading

github-actions bot commented Nov 28, 2021 • edited Loading

WorldSEnder commented Nov 29, 2021

futursolo commented Nov 29, 2021

futursolo commented Nov 30, 2021 • edited Loading

voidpumpkin commented Nov 30, 2021

ranile commented Dec 14, 2021

futursolo commented Dec 15, 2021

ranile commented Dec 16, 2021

futursolo commented Dec 16, 2021

voidpumpkin commented Dec 17, 2021 • edited Loading

Pros

Cons?

voidpumpkin commented Dec 17, 2021

voidpumpkin left a comment

Choose a reason for hiding this comment

futursolo commented Dec 20, 2021

ranile left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

futursolo Dec 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

voidpumpkin left a comment

Choose a reason for hiding this comment

voidpumpkin commented Dec 30, 2021 • edited Loading

futursolo commented Dec 30, 2021 • edited Loading

ranile left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

futursolo commented Nov 28, 2021 •

edited

Loading

futursolo commented Nov 28, 2021 •

edited

Loading

github-actions bot commented Nov 28, 2021 •

edited

Loading

futursolo commented Nov 30, 2021 •

edited

Loading

voidpumpkin commented Dec 17, 2021 •

edited

Loading

futursolo Dec 20, 2021 •

edited

Loading

voidpumpkin commented Dec 30, 2021 •

edited

Loading

futursolo commented Dec 30, 2021 •

edited

Loading