-
Notifications
You must be signed in to change notification settings - Fork 10
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(@netlify/remix-edge-adapter): don't close the stream twice #444
Conversation
✅ Deploy Preview for remix-edge ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for remix-serverless ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
f91f65b
to
a6fb43a
Compare
a6fb43a
to
f953478
Compare
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.
As you say, it's odd that this isn't handled in some way by React - but this solution preserves the handling of abort and doesn't introduce much complexity, so lgtm
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.
Thanks for fixing!
Description
Fixes
TypeError: Closed already requested
that started to be thrown with deno@1.43.0. Suspected underlying reason for change is denoland/deno#23425 and now we receive abort signal after all the work was already completed and response stream is being attempted to close twice (?).TBF, this feels weird that
ReactDOMServer.renderToReadableStream
doesn't handle this in some way to prevent closing stream multiple times. I needed to do tricks with identity TransformStream to figure out state of the stream and preventsignal
from resulting in the error.Alternatives to current fix are those (one of them):
signal
toReactDOMServer.renderToReadableStream
at all (feels wasteful not to be able to abort on demand)console.error
in ouronError
handler so we could filter those out (which would be hacky because it would mean checking error message that is subject to change)Edit:
There is also another error happening (or have been happening):
Which I can't reproduce and also am not seeing it in logs for currently deployed demo site, it's unclear to me wether they are related or not (other than this is can thrown also when attempting to close the stream - https://github.com/denoland/deno/blob/b01578ae1fc1da65ac38daec31a23c9ef652aa74/ext/web/06_streams.js#L5845-L5857 )
Related Tickets & Documents
QA Instructions, Screenshots, Recordings
Compare logs from:
main
- https://app.netlify.com/sites/remix-edge/logs/edge-functions?scope=deployid:66d53a37e9d7480008ad1daf when hitting https://66d53a37e9d7480008ad1daf--remix-edge.netlify.app/There are no error logs produced anymore for each request
For us to review and ship your PR efficiently, please perform the following steps:
ensures we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing a
typo or something that`s on fire 🔥 (e.g. incident related), you can skip this step.
guide and passes our tests.
A picture of a cute animal (not mandatory, but encouraged)