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

feat: Introduce streaming API with Suspense and use. #1630

Merged
merged 31 commits into from
Nov 6, 2023

Conversation

usualoma
Copy link
Member

This PR is based on #1626.

Author should do the followings, if applicable

  • Add tests
  • Run tests
  • yarn denoify to generate files for Deno

@usualoma
Copy link
Member Author

Hi @yusukebe!

There are no significant changes from usualoma#3, but I have added the following three refactorings and a test of the replacement results using "happy-dom".

@usualoma usualoma force-pushed the feat/jsx-streaming branch 2 times, most recently from 09827d0 to 1cda785 Compare October 30, 2023 21:43
@usualoma
Copy link
Member Author

Refactoring is complete.

@yusukebe
Copy link
Member

Hi @usualoma,

I'll take a look this PR along with #1626 and check how it feels to use it. So please give me a minute. Thanks.

@yusukebe
Copy link
Member

yusukebe commented Nov 1, 2023

@usualoma

Is it OK to assume that this PR includes all of #1626? But, if so, let's keep #1626 open until this PR is merged.

@usualoma
Copy link
Member Author

usualoma commented Nov 1, 2023

@yusukebe

Thanks for the confirmation!

I have made a separate PR to clarify the scope of the change, but this PR contains #1626 in its entirety.

We can merge #1626, then this PR, and so on. Or we can merge this PR into the "main" branch (or "next" branch) and close #1626.

Copy link
Member

@yusukebe yusukebe left a comment

Choose a reason for hiding this comment

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

Please update package.json for hono/jsx/streaming:

  "exports": {
    //...
    "./jsx/streaming": {
      "types": "./dist/types/jsx/streaming.d.ts",
      "import": "./dist/jsx/streaming.js",
      "require": "./dist/cjs/jsx/streaming.js"
    },
  "typesVersions": {
    "*": {
      //...
      "jsx/streaming": [
        "./dist/types/jsx/streaming.d.ts"
      ],

src/jsx/streaming.ts Outdated Show resolved Hide resolved
src/jsx/streaming.ts Show resolved Hide resolved
src/jsx/streaming.ts Outdated Show resolved Hide resolved
src/jsx/streaming.ts Show resolved Hide resolved
@yusukebe
Copy link
Member

yusukebe commented Nov 4, 2023

Hi @usualoma!

Sorry for the late reply. I've tested this feature thoroughly and it's amazing!

The example below demonstrates it working seamlessly:

import { Hono } from 'hono'
import { use, Suspense, renderToReadableStream } from 'hono/jsx/streaming'
import type { Shop } from './types'

const app = new Hono()

const fetchData = async (): Promise<{ shop: Shop }> => {
  const res = await fetch('https://ramen-api.dev/shops/yoshimuraya')
  return res.json()
}

const AsyncComponent = () => {
  const data = use(fetchData())
  return <p>I like {data.shop.name} 🍜</p>
}

app.get('/', async (c) => {
  const stream = renderToReadableStream(
    <html>
      <body>
        <h1>SSR Streaming</h1>
        <Suspense fallback={<p>loading...</p>}>
          <AsyncComponent />
        </Suspense>
      </body>
    </html>
  )
  return c.body(stream, {
    headers: {
      'Transfer-Encoding': 'chunked',
      'Content-Type': 'text/html; charset=UTF-8'
    }
  })
})

export default app

Additionally, with this PR honojs/vite-plugins#19, the @hono/vite-dev-server now supports streaming, allowing us to develop an app utilizing this feature alongside Vite.

I've left some comments on the PR, please check them out.

I believe the streaming methods: use, Suspense, and renderToReadableStream should be tagged as experimental. Although they might not be as frequently used as other features, they indeed highlight our capabilities.

@usualoma
Copy link
Member Author

usualoma commented Nov 5, 2023

Hi @yusukebe!
Thanks for your comments and advice. I have added a commit to each of your points.

b91a87d

Basically, exceptions should be caught by the application and not by the framework, but if we don't catch them, streaming will not be closed, so we catch them.

I think the test is OK, but when I add this test, the vitest catches the global unhandledRejection and makes an error, so I now skip it.

https://github.com/vitest-dev/vitest/blob/7f5022994ec645838d9fa1867602b2f30d51673a/packages/browser/src/client/main.ts#L104

https://github.com/honojs/hono/pull/1630/files#diff-73983199d8fca23993d81d61ae2256197fce29b2b2855d6ea10dbf57d7283f15R123

Demo app

Your demo app is simple and great. It works well. However, the current code calls fetch() needlessly, so I think it would be better to change it a little. (Sorry if you knew what you were doing...)

As is the case with React, the following code will display "fetchData" multiple times.

import React, { Suspense, use } from "react";
import { renderToReadableStream } from "react-dom/server";

const fetchData = async () => {
  await new Promise((resolve) => setTimeout(resolve, 10));
  console.log("fetchData");
  return "OK";
};

function Component() {
  const res = use(fetchData());
  return <p>${res}</p>;
}

const elm = (
  <Suspense fallback={<div>Loading...</div>}>
    <Component />
  </Suspense>
);

const stream = await renderToReadableStream(elm);
const decoder = new TextDecoder("utf-8");
for await (const chunk of stream) {
  console.log(decoder.decode(chunk));
}
image

For example, if the result of fetch() is cached as a Promise as shown below, unnecessary fetch() will not be called.

import { Hono } from 'hono'
import { use, Suspense, renderToReadableStream } from 'hono/jsx/streaming'
import type { Shop } from './types'

const app = new Hono()

const fetchDataCache = {}
const fetchData = (name): Promise<{ shop: Shop }> =>
  (fetchDataCache[name] ||= fetch(`https://ramen-api.dev/shops/${name}`).then((res) => res.json()))

const AsyncComponent = () => {
  const data = use(fetchData("yoshimuraya"))
  return <p>I like {data.shop.name} 🍜</p>
}

app.get('/', async (c) => {
  const stream = renderToReadableStream(
    <html>
      <body>
        <h1>SSR Streaming</h1>
        <Suspense fallback={<p>loading...</p>}>
          <AsyncComponent />
        </Suspense>
      </body>
    </html>
  )
  return c.body(stream, {
    headers: {
      'Transfer-Encoding': 'chunked',
      'Content-Type': 'text/html; charset=UTF-8',
    },
  })
})

export default app

@yusukebe
Copy link
Member

yusukebe commented Nov 5, 2023

Hi @usualoma,

Thanks. I understood both.

Additionally, something awesome happened last night. One of the React members, Dan, mentioned the honojs account on Twitter and gave us an advice. It's an honor to receive an advice from a React committer regarding JSX.

https://x.com/dan_abramov/status/1721179995370914192?s=20
https://x.com/dan_abramov/status/1721230099368841219?s=20

I agree with what he said; the use() is not necessary for the API. We can simplify it by removing use():

const AsyncComponent = async () => {
  const res = await fetch(`https://ramen-api.dev/shops/yoshimuraya`)
  const data = await res.json()
  return <p>I like {data.shop.name} 🍜</p>
}

app.get('/', async (c) => {
  const stream = renderToReadableStream(
    <html>
      <body>
        <h1>SSR Streaming</h1>
        <Suspense fallback={<p>loading...</p>}>
          <AsyncComponent />
        </Suspense>
      </body>
    </html>
  )
  // ...
})

This is enabled if the Async Component inside Suspense is treated as a streaming component.

What do you think about this? If we can implement it, I believe we won't need to create use().

@usualoma
Copy link
Member Author

usualoma commented Nov 6, 2023

Hi @yusukebe!

Oh, that is a great honor and much-appreciated advice!
I didn't know that even in React, if we use the Async Component in Suspense, it is treated as a streaming component without using use(). If so, it certainly makes more sense not to use use() in hono.

I implemented that in f4589b7.
I removed use() in 1573ec1; or if we keep it, users who used use() in React may be able to migrate to it with little learning cost.

@yusukebe
Copy link
Member

yusukebe commented Nov 6, 2023

@usualoma

Thanks!

I didn't know that even in React, if we use the Async Component in Suspense, it is treated as a streaming component without using use(). If so, it certainly makes more sense not to use use() in hono.

He might have been referring only to the hono/jsx spec, and not React, as he knows our hono/jsx doesn't follow the React spec completely. In React, it's still necessary to throw a Promise in Suspense, and maybe it should support client components too (although I don't have took a look the spec well).

Anyway. All things are done. I'll prepare the "next" branch and I'll merge this into it.

@usualoma
Copy link
Member Author

usualoma commented Nov 6, 2023

@yusukebe

Uh, I wrote the code to make sure it worked. (I haven't read the code for React's internal implementation though.)

The following code using React's Suspense is handled as streaming content in SSR.

import { Hono } from "hono";
import React, { Suspense } from "react"; // 18.3.0
import { renderToReadableStream } from "react-dom/server";

const app = new Hono();

async function Component() {
  const data = await (
    await fetch("https://ramen-api.dev/shops/takasagoya")
  ).json();
  await new Promise((resolve) => setTimeout(resolve, 1000));
  return <h1>{data.shop.name}</h1>;
}

app.get("/", async (c) => {
  const stream = renderToReadableStream(
    <html>
      <head>
        <meta charSet="utf-8" />
      </head>
      <body>
        <Suspense fallback={<div>Loading...</div>}>
          <Component />
        </Suspense>
      </body>
    </html>
  );

  return c.body(await stream, 200, {
    "X-Content-Type-Options": "nosniff",
    "Content-Type": "text/html",
  });
});

export default app;
ssrstreaming.mov

Therefore, the fact that async components are treated as streaming content in Suspense is not a specification unique to hono, but a behavior compatible with the original React.

@yusukebe
Copy link
Member

yusukebe commented Nov 6, 2023

@usualoma

I got it! As you said, this is good thing for React user to migrate to hono/jsx. Great!

@yusukebe yusukebe changed the base branch from main to next November 6, 2023 22:05
@yusukebe
Copy link
Member

yusukebe commented Nov 6, 2023

It's a time for merging! I'll merge this into the "next" branch and release a RC version first. Thanks!

@yusukebe yusukebe merged commit fedeb3d into honojs:next Nov 6, 2023
11 checks passed
@yusukebe
Copy link
Member

yusukebe commented Nov 7, 2023

@usualoma

Wow! https://x.com/dan_abramov/status/1721811659453313480?s=20

@usualoma usualoma deleted the feat/jsx-streaming branch November 7, 2023 10:27
@shellscape
Copy link

shellscape commented Nov 7, 2023

@yusukebe I'm testing this out and having some TypeScript trouble with the latest example above. I've got

    "jsx": "react",
    "jsxFactory": "jsx",
    "jsxFragmentFactory": "Fragment"

in my tsconfig.json file, but JSX components aren't being recognized. Any suggestions?

Update: I am a silly person and did not use a .tsx file extension

@usualoma usualoma mentioned this pull request Nov 7, 2023
3 tasks
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.

3 participants