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

Fix top level async breaks with routes that don't stream #2280

Open
wants to merge 1 commit into
base: v1.x-2022-07
Choose a base branch
from

Conversation

blittle
Copy link
Contributor

@blittle blittle commented Oct 28, 2022

Description

Fix a critical issue where a top level asynchronous request within App.server.jsx would break if subsequent routes attempt to disable streaming. For example:

// App.server.jsx
export default renderHydrogen() {
  useSession(); 
  ...
}

// NotFound.server.jsx
export default function NotFound({response}) {
  resonse.doNotStream();
  ...
}

Additional context


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

  • 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)
  • Update docs in this repository according to your change
  • Run yarn changeset add if this PR cause a version bump based on Keep a Changelog and adheres to Semantic Versioning

@@ -417,7 +419,7 @@ async function runSSR({

log.trace('start ssr');

const rscReadable = response.canStream()
const rscReadable = canStream
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem starts here, the rscReadable is setup based on the initial status of streaming. But if something suspends in App.server.jsx, this will always be true. Later in the logic down below though all of a sudden response.canStream() is false. So to keep it consistent, the variable is set once and reused everywhere.

(resp) => resp.text()
);

expect(response).toContain('<meta data-flight="S2');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The case where it breaks, <meta data-flight actually is embedded twice. So the value won't be S2, rather an encoded <meta data-flight

Copy link
Contributor

@jplhomer jplhomer left a comment

Choose a reason for hiding this comment

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

Nice find and fix 👍

But just to clarify: If you suspend in the root App component, then any calls to response.doNotStream() in route files will not be effective. Are we aligned there?

This is what we have in our documentation: https://shopify.dev/custom-storefronts/hydrogen/framework/routes#response-donotstream

Copy link
Collaborator

@wizardlyhel wizardlyhel left a comment

Choose a reason for hiding this comment

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

good find

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