-
Notifications
You must be signed in to change notification settings - Fork 47.9k
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
[Flight] Better compat with http.createServer #17289
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit c36a1b3:
|
Size changes (stable)ReactDOM: size: 0.0%, gzip: -0.2% Details of bundled changes.Comparing: 62ef250...c36a1b3 react-dom
|
Size changes (experimental)ReactDOM: size: 0.0%, gzip: -0.0% Details of bundled changes.Comparing: 62ef250...c36a1b3 react-dom
|
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.
Seems unfortunate to do these checks in the hot path but ok for now I guess.
I also wonder if the HTTP server always flushes at the end of the tick or what it does for buffering.
Would be bad if it waits for a few bytes that takes long to load before flushing the previous pieces.
I guess we'll see what objects returned by Express etc are like. Maybe they shield from this and we won't need it. |
Don't know if we care in longer term but I use
http.createServer
for testing and that has older Node streams. They don't havecork
anduncork
. Also theflush
on that object was misleading (because it was only used for flushing headers) so it fires a deprecation warning.That's a few workarounds to make it work with
http.createServer
. Maybe we'll remove it later but that's the most convincing way to "really" test it that I found so far.