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.

Sign up for GitHub

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

Jump to bottom

feat: support streaming in Workers runtimes where streaming is supported #326

Closed

jplhomer wants to merge 1 commit into main from jl-stream-workers

Contributor

jplhomer commented Dec 3, 2021 •

edited

Loading

Description

This PR updates handle-event and entry-server logic to support streaming in Workers runtimes where ReadableStream is supported.

Previously, we only streamed responses back to Node.js runtimes where we had a pipeable response object.

Fixes #101

Additional context

A lot going on here! Here's a breakdown of what's happening:

We update the isStreamable logic in handle-event to account for supporting ReadableStream
We now return the values of stream and hydrate
In entry-server, we refactor bodies of stream and hydrate to call streamToWorkerResponse and streamToNodeResponse
In each of streamToWorkerResponse and streamToNodeResponse, we add various checks and forks to account for variability in response types. For example, we need to allow a developer to prevent streaming in order to return a custom page response. And for /react hydration requests, we need to buffer the responses and then pipe them through our wire syntax converter before responding.

Note: There is an open React bug which prevents proper streaming in workers runtimes. We'll want to hold off on shipping this until that is resolved. facebook/react#22772

Note: Oxygen technically supports ReadableStream via polyfill, but official streaming is not yet supported. We need to find out if shipping this before official support lands will cause issues. E.g. will it just buffer and return the response like normal? cc @maxshirshin UPDATE: I tested it with oxygen-run and It works just fine 🔥 😎

🎩 Tophat instructions

cd packages/playground/server-components-worker
yarn build && yarn dev

Notice that the contents stream in real fast 🔥

To spice things up, you should update the index.server.jsx component to make a query and witness the Suspense fallback on the page as the content streams in:

import {Link, useShopQuery} from '@shopify/hydrogen';

export default function Index() {
  const {data} = useShopQuery({query: `query { shop { name } }`});

  return (
    <>
      <h1>Home: {data.shop.name}</h1>
      <Link className="btn" to="/about">
        About
      </Link>
    </>
  );
}

Before submitting the PR, please make sure you do the following:

Add your change under the Unreleased heading in the package's CHANGELOG.md
Read the Contributing Guidelines
Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123)


          feat: support streaming in Workers runtimes where streaming is supported

253b688

jplhomer requested a review from a team

December 3, 2021 20:23

blittle reviewed

View reviewed changes

Contributor

blittle left a comment

🚀 amazing work! 🚀

packages/hydrogen/src/entry-server.tsx

+                        response,
+                        head,
+                        state,
+                        dev,

Contributor

blittle Dec 3, 2021

Nit picky, but maybe explicitly pass isReactHydrationRequest: false here, just to make it more readable.

packages/hydrogen/src/entry-server.tsx

-                        writer.drain();
+                  const stream = renderToReadableStream(app, {
+                    onReadyToStream() {
+                      canStream = !isReactHydrationRequest && componentResponse.canStream();

Contributor

blittle Dec 3, 2021

Will the hydration request ever be streamable? (down the road)

Contributor

frandiox Dec 6, 2021

Yes, it will, but we need to call a different renderToReadableStream (like this one).

packages/hydrogen/src/entry-server.tsx

-                        console.error(error);
+                  async function maybeSendResponse() {
+                    if (!canStream) {
+                      setTimeout(maybeSendResponse, 20);

Contributor

blittle Dec 3, 2021

In the case where isReactHydrationRequest === true, will it indefinitely keep setting the timeout?

Contributor

maxshirshin Dec 6, 2021

Do we need to handle the case when error is not undefined (i.e. when the onError callback fires?).

packages/hydrogen/src/entry-server.tsx

+;
+                    /**
+                     * Don't allow custom statuses for Hydration responses.

Contributor

blittle Dec 3, 2021

This comment is confusing to me, it looks like the if block is changing the status for hydration requests

Contributor

blittle Dec 3, 2021

I think the comment maybe should be moved to line 225?

frandiox reviewed

View reviewed changes

packages/hydrogen/src/entry-server.tsx

+                    );
+                  },
+                  onCompleteAll() {
+                    if (!isReactHydrationRequest) {

Contributor

frandiox Dec 6, 2021

Just a suggestion but, since we will need to use a different pipeToNodeWritable and renderToReadableStream for RSC response at some point, perhaps we can extract all of these "isReactHydrationRequest" branches already in separate functions? To make the refactoring easier later I mean 🤔

maxshirshin reviewed

View reviewed changes

packages/hydrogen/src/entry-server.tsx

-                      onError(error: any) {
-                        didError = error;
-                        console.error(error);
+                  async function maybeSendResponse() {

Contributor

maxshirshin Dec 6, 2021

Despite the explanation above, I personally find the pattern used here to be quite confusing. There's no clear indication of what makes the magic "work", that may complicate code maintenance.

Could we try using waitUntil if it's supported instead? or maybe abstract this code as a CF-specific implementation? In either case, if this code is inspired by the need to find a workaround for a weird runtime behaviour, it probably shouldn't be the default since it may trigger other corner cases in other environments. And, afaik, CF can't currently instantiate ReadableStreams, so this may only run in CF once that support is added, which may also change the underlying implementation and remove the need for the hack.

Contributor

maxshirshin commented Dec 6, 2021

@jplhomer Oxygen currently reads Response instances by calling their .text() method and awaiting for the returned promise to settle, so as long as the .text() method works correctly with the underlying streaming source, we should be fine.

maxshirshin reviewed

View reviewed changes

packages/hydrogen/src/framework/runtime.ts

@@ @@ -42,3 +42,12 @@ export function runDelayedFunction(fn: () => Promise<any>) { @@
                 return context.waitUntil(fn());
               }
+              export function supportsReadableStream() {

Contributor

maxshirshin Dec 10, 2021

We must avoid the situation where Hydrogen ships streaming support assuming that Oxygen reads responses using non-streaming APIs, and Oxygen ships its streaming support assuming that Hydrogen never streams. Even if it works correctly on both sides, there might be performance implications. Let's connect to discuss putting this behind a feature-flag in Oxygen (now that we have Oxygen namespace)?

Contributor

maxshirshin Dec 10, 2021

Added https://github.com/Shopify/custom-storefronts/issues/161

Contributor Author

jplhomer commented Jan 21, 2022

Closing this in favor of Shopify/hydrogen#498

jplhomer closed this

jplhomer deleted the jl-stream-workers branch

January 21, 2022 16:14

rafaelstz pushed a commit to rafaelstz/hydrogen that referenced this pull request


          Merge pull request Shopify#326 from Shopify/jl-tmp-include-templates-…

781bafc

…in-cli-dist

Copy templates into CLI dist folder

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet