Improve the Fiber and Model stream context #312

pedrobslisboa · Oct 11, 2025

Description

This PR refactors the way we handle the context for Model and Fiber. Instead of 2 different contexts for the Stream, we will have the same context type for both.

type 'a context = {
  push : 'a -> unit;
  close : unit -> unit;
  mutable index : int;
  mutable pending : int;
  env : env;
  debug : bool;
  }

The context controls async and sync push as the index and pending, so we no longer need to worry about it. We declare the value to push and use push/push_async, and the context will handle the rest:

  let push chunk ~context =
    let index = context.index in
    context.push (chunk index);
    context.index <- context.index + 1;
    index

  let push_async promise ~context =
    let index = context.index in
    context.index <- context.index + 1;
    context.pending <- context.pending + 1;
    Lwt.async (fun () ->
        let%lwt chunk = promise in
        context.pending <- context.pending - 1;
        context.push (chunk index);
        if context.pending = 0 then context.close ();
        Lwt.return ());
    index

The concept of inversion of control was applied at render_element_to_html and client_values_to_json. Then we can use the client_values_to_jsonatrender_element_to_html, which reduces the amount of code to maintain, also keeping it way simpler to understand:

Before

After

| Client_component { import_module; import_name; props; client } ->
    let context = Fiber.get_context fiber in
    let lwt_props =
      Lwt_list.map_p
        (fun (name, value) ->
          match (value : React.client_value) with
          | Element element ->
              let%lwt _html, model = render_element_to_html ~fiber element in
              Lwt.return (name, model)
          | Promise (promise, value_to_json) ->
              let index = Fiber.use_index fiber in
              let async : Html.element Lwt.t =
                let%lwt value = promise in
                let json = value_to_json value in
                let ret = chunk_script (Model.model_to_chunk index json) in
                Lwt.return ret
              in
              context.pending <- context.pending + 1;
              Lwt.async (fun () ->
                  let%lwt html = async in
                  context.push html;
                  context.pending <- context.pending - 1;
                  if context.pending = 0 then context.close ();
                  Lwt.return ());
              Lwt.return (name, `String (Model.promise_value index))
          | Json json -> Lwt.return (name, json)
          | Error error ->
              let index = Fiber.use_index fiber in
              let error_json =
                Model.make_error_json 
                     ~env:context.env 
                     ~stack:error.stack 
                     ~message:error.message 
                     ~digest:error.digest
              in
              context.push (error_chunk_script index error_json);
              Lwt.return (name, `String (Model.error_value index))
          | Function action ->
              let index = Fiber.use_index fiber in
              let html =
                chunk_script 
                         (Model.model_to_chunk index 
                         (`Assoc [ ("id", `String action.id); ("bound", `Null) ]))
              in
              context.push html;
              Lwt.return (name, `String (Model.action_value index)))
        props
    in
    let lwt_html = client_to_html ~fiber (client ()) in
    let index = Fiber.use_index fiber in

    let ref : json = Model.component_ref 
                                 ~module_:import_module 
                                 ~name:import_name in
    context.push (client_reference_chunk_script index ref);
    let%lwt html, props = Lwt.both lwt_html lwt_props in
    let model = Model.node ~tag:(Model.ref_value index) ~props [] in
    Lwt.return (html, model)

| Client_component { import_module; import_name; props; client } ->
    let context = get_context fiber in
    let props =
      let on_push value = on_push (Model value) in
      Model.client_values_to_json ~context:fiber.context ~on_push props
    in
    let%lwt html = client_to_html ~fiber ~on_push (client ()) in
    let ref : json = Model.component_ref 
                                 ~module_:import_module 
                                 ~name:import_name in
    let index = Stream.push ~context (on_push (Model (Component_ref ref))) in
    let model = Model.node ~tag:(Model.ref_value index) ~props [] in
    Lwt.return (html, model)

davesnx

Great job

davesnx · Oct 11, 2025

packages/reactDom/test/test_RSC_model.ml

-      "1:[\"$\",\"span\",null,{\"children\":\"hi\"},null,[],{}]\n";
-      "2:[\"$\",\"span\",null,{\"children\":\"hola\"},null,[],{}]\n";
-      "0:[\"$1\",\"$2\"]\n";


are these changes "on purpose"?

davesnx · Oct 11, 2025

packages/reactDom/test/test_RSC_model.ml

+      "2:D\"$1\"\n";
      "0:[\"$\",\"input\",null,{\"id\":\"sidebar-search-input\",\"placeholder\":\"Search\",\"value\":\"my \


whoa, this was a bug?

davesnx · Oct 11, 2025

packages/reactDom/src/ReactServerDOM.ml

+  let push_async promise ~context =
+    let index = context.index in
+    context.index <- context.index + 1;
+    context.pending <- context.pending + 1;
+    Lwt.async (fun () ->
+        let%lwt chunk = promise in
+        context.pending <- context.pending - 1;
+        context.push (chunk index);
+        if context.pending = 0 then context.close ();
+        Lwt.return ());
+    index


Looks like a decent abstraction, but caused some tests to behave differently. (since we pushed a few things instead of push once)

Just solved some of those issues. I'm going to look better next week at the test cases.

davesnx · Oct 11, 2025

packages/reactDom/src/ReactServerDOM.ml

-
 module Model = struct
-  type chunk_type = Chunk_value of json | Chunk_component_ref of json | Debug_info_map of json | Chunk_error of json
+  type t = Value of json | Debug_info_map of json | Component_ref of json | Error of env * React.error


Chunk_value and Chunk_component_ref had different serialisation right? How come we join them?

Never mind the diff confused me. Chunk_value -> Value looks... interesting?

You know, I was wondering if Json is better. Like, "push Json value".

Model.chunk is good type name btw, if you go with chunk, then the type make sense: value | component_ref | debug_info

davesnx · Oct 11, 2025

packages/reactDom/src/ReactServerDOM.ml

-                    context.push id (Chunk_value (`Assoc [ ("id", `String f.id); ("bound", `Null) ]));
-                    React.JSX.String (key, key, action_value id)
+                    let index =
+                      Stream.push ~context (on_push (Value (`Assoc [ ("id", `String f.id); ("bound", `Null) ])))


Stream.push ~context onpush?

shouldn't be just "on_push"?

davesnx · Oct 11, 2025

packages/reactDom/src/ReactServerDOM.ml

-      | Chunk_component_ref json -> push (client_reference_to_chunk id json)
-      | Chunk_error json -> push (error_to_chunk id json)
+module Fiber = struct
+  type chunk = Model of Model.t | Suspense_Boundary of Html.element | Batch of chunk list


how's possible that chunk contains Suspense_boundary (boundary could be lowercase) but context can stream Html.element?

This looks like an extra diff from the refactor?

davesnx · Oct 11, 2025

packages/reactDom/src/ReactServerDOM.ml

+    match chunk with
+    | Model model -> model_to_chunk model id
+    | Suspense_Boundary str -> boundary_to_chunk id str
+    | Batch chunks -> chunks |> List.map (fun chunk -> to_chunk chunk id) |> Html.list


When Batch is needed?

davesnx · Oct 12, 2025

packages/reactDom/src/ReactServerDOM.ml

-  let%lwt root_html, root_model = render_element_to_html ~fiber element in
-  let root_chunk = client_value_chunk_script initial_index root_model in
-  context.pending <- context.pending - 1;
+  let%lwt root_html, root_model = Fiber.render_element_to_html ~fiber ~on_push:Fiber.to_chunk element in


If we send fiber, why send on_push as a labelled arg?

davesnx · Oct 12, 2025

packages/reactDom/src/ReactServerDOM.ml

  in
-  let stream, push, close = Push_stream.make () in
-  let context : Fiber.context = { push; close; pending = 1; index = initial_index; env } in
+  let stream, context = Stream.make ~env ~debug:false ~initial_index:1 in


Why the initial index starts at 1?

davesnx · Oct 12, 2025

packages/reactDom/src/ReactServerDOM.ml

-      | Debug_info_map json -> push (debug_info_to_chunk id json)
-      | Chunk_component_ref json -> push (client_reference_to_chunk id json)
-      | Chunk_error json -> push (error_to_chunk id json)
+module Fiber = struct


Fiber got a lot of line movements that makes it a bit hard to review the diff and I only see one commit, could we maybe go step by step in person?

pedrobslisboa requested a review from davesnx October 11, 2025 17:40

pedrobslisboa self-assigned this Oct 11, 2025

pedrobslisboa added the enhancement New feature or request label Oct 11, 2025

pedrobslisboa mentioned this pull request Oct 11, 2025

Allow async into client components #310

Draft

refact: improve the Fiber and Model stream context

0eb4aea

pedrobslisboa force-pushed the feat/fiber-model-ctx branch from 17f4e18 to 0eb4aea Compare October 11, 2025 17:44

davesnx reviewed Oct 12, 2025

View reviewed changes

pedrobslisboa force-pushed the feat/fiber-model-ctx branch from 8c150ae to 12df513 Compare October 17, 2025 07:29

feat: improve debugging due to new fiber/model contextg

e3adf79

pedrobslisboa force-pushed the feat/fiber-model-ctx branch from 12df513 to e3adf79 Compare October 17, 2025 07:33

		"2:D\"$1\"\n";
		"0:[\"$\",\"input\",null,{\"id\":\"sidebar-search-input\",\"placeholder\":\"Search\",\"value\":\"my \

Search code, repositories, users, issues, pull requests...

Uh oh!

Improve the Fiber and Model stream context #312

Are you sure you want to change the base?

Improve the Fiber and Model stream context #312

Uh oh!

Conversation

pedrobslisboa commented Oct 11, 2025

Description

Uh oh!

davesnx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants