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

Suspense boundaries/out of order streaming/anyhow like error handling #2365

Merged
merged 227 commits into from
Jul 2, 2024

Conversation

ealmloff
Copy link
Member

@ealmloff ealmloff commented Apr 24, 2024

This PR improves context based suspense by bubbling suspense to the nearest suspense boundary.

This allows you to handle suspense at a single boundary which makes it easier to show an unified loading UI.

It uses the current extension trait to return early when a suspended resource is encountered, but this now returns an error instead of None to avoid issues when the suspense is handled manually in the component.

// Suspense context
#[component]
fn Parent() -> Element {
    rsx! {
        ErrorBoundary {
            handle_error: |err| rsx! { div { {err} } },
            Suspense {
                pending: |suspense| rsx! { div { {suspense} } },
                Doggo {}
            }
        }
    }
}

#[component]
fn Doggo() -> Element {
    let download_progress = use_signal(|| 0.);
    let mut fut = use_resource(move || async move { download_model(download_progress).await })
        .suspend()
        // You can optionally add a hint for the suspense boundary to render
        .with_suspense_placeholder(move || rsx!("Downloading LLM... {download_progress}% done"))?
        .throw()
        // You can optionally add a hint for the error boundary to render
        .show_with(|err| rsx!("hugging face is down 🙁: {err:?}"))?;

    todo!()
}

Breaking changes:

  • This PR changes the Element type from Option to Result to make it easier to return errors early, and properly track the result of the component. Without this change, if you throw or suspend, handle it manually and then return None manually, dioxus will try to handle the suspense/error for you

Closes #1335
Closes #2389
Closes #2264

Stacked on top of #2226, #2437, #2444

@ealmloff ealmloff added enhancement New feature or request core relating to the core implementation of the virtualdom breaking This is a breaking change fullstack related to the fullstack crate labels Apr 24, 2024
@jkelleyrtp
Copy link
Member

Need to do:

  • Set { target: “es2015” } in tsconfig to use modern web features
  • More comments on struct fields that have ominous names
  • Basepath should be env! so we dont maintain the weird binary string parsing code
  • Use the interpreter for hydrating templates - not comments + template node walking
    • the interpreter has all the info we need about which nodes are “live” and connected to suspense
    • Get rid of the onremove callback because we’ll use interpreter for roots (more robust)

Suggestions:

  • Maybe change name on vnode::empty to Vnode::None/none?
  • Default features should be off for most/all deps in dioxus - desktop is the only one with default-features=true
  • Cfg-ing fields on the trait vs trait itself - why do we cfg out each method on file engine?
  • Maybe dont send scripts down with each chunk - one script that handles all future chunks?
  • To explore: writing directly to buffer without intermediate string
  • streaming.rs should maybe be in fullstack?
  • Maybe using vecs instead of opcodes in the String cache?
  • Some places where type inference goofs (serialization of error type?)
  • Can we collapse the two error types into one?
  • Can we improve error handling + type inference around use_server_future

Future Thoughts:

  • Inspect token respanning for ctrl-click - currently every token gets the same span which confuses RA
  • We should move owners into core such that we don’t generate the confusing *Owner helper struct
  • Maybe we put file engine in one place only - also it’s maybe broken on mobile (std::fs on mobile does what?)
  • We should snip unused js snippets/websys code eventually.
  • You should not be able to set a children field that isn’t children in core macro
  • No more .dioxus folder
  • To explore: holding futures longer for seo on streaming - suspend on component? - support fully loaded page for crawlers
  • Eventually switch to comment nodes for placeholders
  • Key is not a real ident and does not support dosc/cmd-click (jon fix that?)

@ealmloff ealmloff requested a review from jkelleyrtp July 2, 2024 00:48
Copy link
Member

@jkelleyrtp jkelleyrtp left a comment

Choose a reason for hiding this comment

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

lgtm

@jkelleyrtp jkelleyrtp merged commit 022e4ad into DioxusLabs:main Jul 2, 2024
10 checks passed
@ealmloff ealmloff deleted the suspense-2.0 branch July 2, 2024 03:52
@ealmloff
Copy link
Member Author

ealmloff commented Jul 2, 2024

Some notes from discussion during review:

  • Suspense is kind of odd as a component; It shares none of the state with components and shares none of the diffing logic with components. It was convenient to implement it this way initially because components already have arbitrary state. Scopes also let us communicate up to parent components and trigger the scope to rerun. It might be better to structure suspense boundaries as a DynamicNode?
  • Hydration logic lives in a lot of different places:
    • Web owns rebuilding the dom for hydration and hydrating data
    • The interpreter owns hydrating real dom nodes with a list of elementids
    • Fullstack owns collecting hydration data for the client and streaming hydration chunks
    • SSR owns rendering hydration ids for web to rehydrate with
      Each of these components are difficult to test independently in a way that isn't very flakey. Eg. we could test ssr renders <!--nodeid12-->, but we really only care about dioxus web knowing what that 12 means, not the contents itself
  • The schedulers and renders are very codependent in a way that isn't well documented/tested. Fullstack expects the renderer to know what scopes to poll and when and the scheduler tells fullstack what scopes it needs to send to the client. Could be interesting to invert control here. If the scheduler drives rendering, can it own the main loop?
  • We use some generated code in core that should just be the component macro
  • Fullstack supports many different rendering modes: SSR, SPA, SSG, ISG, and streaming. Out of those ISG and SSG can just be a caching layer on top of Streaming and SSR and SPA are both one side of the full streaming renderer
  • Core integrates more deeply with web specific logic for suspense than any other feature. We hide the APIs, but it would be nice if this was pulled out of core somehow or generalized

@jkelleyrtp
Copy link
Member

More thoughts

Random thoughts:

  • SSR/ISRG/SPA/SSG/Streaming are all super hard to grok
    • Too many options
    • If I’m building an app, what do I want? How do I choose?
    • For the average developer, we want the entrypoint to be simple: web for all client needs and server for all server needs.
    • We almost don’t want to officially expose things like SSR/SSG as options since they will get confusing.
    • However, server gets confusing: do I need a server? When does the server come into play?
    • What is a website??
    • to revisit later…
  • where does the logic for all the streaming/hydration/ssr/isrg stuff live?
    • caching being in SSR is kinda confusing for the few developers that try to use dioxus in ssr mode.
    • SSR in the eyes of a developer is kinda an entrypoint for server stuff. But pushing the ssr feature onto people is confusing since there’s not much in there they actually want. What you really want is fullstack but the names are all gross right now
    • Streaming is split apart into too many files. IDs, serialization, suspense, etc. Streaming needs to reach into core to handle suspense boundaries flipping around which just makes it annoying to deal with.
    • Testing only with integration tests is a pain. Ideally we have ways to compartmentalize the little bits of the logic to ensure things like mounted events are queued, layout_effect, etc. as we add more details.
    • Currently feels like a shaky ground - not super stable to add more features to the system. There are several cross-cutting config/caching bits in SSR that feel like they would be better in the fullstack crate.
    • We need to come up with a better fullstack harness so getting as much complex logic colocated into one crate would be nice if we do end up building custom harnesses there (bun run? more playwright?)
  • streaming pulls on wayyyyy too many pieces
    • custom code in web
    • custom code in interpreter
    • custom code in core (via suspense boundary props and a doc(hidden) flag)
    • fullstack relies on the interpreter now to supply it some javascript that’s just not great
    • ^ is because fullstack also serves your app
  • Why are we throwing chunks into your app when we have the data to also render those chunks?
    • Can’t we just call render on the components once we have the data?
    • Evan says: SEO
      • Can we just serve fully-formed pages to crawlers?
    • Evan says: performance
      • 🤷‍♂️ maybe I guess?
    • Evan says: the chunks are there, we needed them anyway
      • We wouldn’t need to SSR the chunks in the first place… if we’re gonna send down server data might as well
    • Evan says: total complexity added here is 20% of the PR
      • Would be nice to not have this additional complexity if it necessitates so much from core.
      • How much of the cross-cutting logic here is because of chunk throwing? Multiple IDs?
    • We could also move the hydrate logic into core or something such that we’re not reaching around to do what is essentially hydration
    • Almost seems like sending data down is more reliable than chunk throwing
  • Suspense in the virtualdom
    • Why not just progress state without the Option?
    • We use the Option to not have monomorphization kick in
    • Is implemented via props override
      • Can we just tack it on as “data” to the components and check that rather than downcasting the props type constantly?
      • Could be a new DynamicNode
    • Requires us to move the “Root” Scope from 0 to 3 - gross! This is not an important assumption per se but does get in the way of purity. Why are we auto-inserting suspense boundaries? Error boundaries too? React doesn’t do that.
      • We could insert them in dev mode or something and then yell at you that you should insert them yourself.
      • We also default to crashing your app top-down by default instead of letting it sit in a dead state
      • Our crash state isn’t great, it’s an actual complete irrecoverable crash
  • Logic for cross-cutting features
    • use_server_future needs to throw data to the client
    • How is this organized? Can we make it cleaner? Easier to test
  • Not a fan of putting stuff in the window

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking This is a breaking change core relating to the core implementation of the virtualdom enhancement New feature or request fullstack related to the fullstack crate
Projects
None yet
2 participants