-
Notifications
You must be signed in to change notification settings - Fork 325
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
base: v1.x-2022-07
Are you sure you want to change the base?
Conversation
@@ -417,7 +419,7 @@ async function runSSR({ | |||
|
|||
log.trace('start ssr'); | |||
|
|||
const rscReadable = response.canStream() | |||
const rscReadable = canStream |
There was a problem hiding this comment.
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'); |
There was a problem hiding this comment.
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
There was a problem hiding this 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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good find
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:Additional context
Before submitting the PR, please make sure you do the following:
fixes #123
)yarn changeset add
if this PR cause a version bump based on Keep a Changelog and adheres to Semantic Versioning