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

[Fizz] handle errors in onHeaders #27712

Merged
merged 1 commit into from
Nov 15, 2023
Merged

Conversation

gnoff
Copy link
Collaborator

@gnoff gnoff commented Nov 15, 2023

onHeaders can throw however for now we can assume that headers are optimistic values since the only things we produce for them are preload links. This is a pragmatic decision because React could concievably have headers in the future which were not optimistic and thus non-optional however it is hard to imagine what these headers might be in practice. If we need to change this behavior to be fatal in the future it would be a breaking change.

This commit adds error logging when onHeaders throws and ensures the request can continue to render successfully.

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Nov 15, 2023
@react-sizebot
Copy link

react-sizebot commented Nov 15, 2023

Comparing: aec521a...0ad7cc5

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js = 175.90 kB 175.90 kB = 54.75 kB 54.76 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 177.97 kB 177.97 kB = 55.39 kB 55.39 kB
facebook-www/ReactDOM-prod.classic.js = 569.81 kB 569.81 kB = 100.29 kB 100.29 kB
facebook-www/ReactDOM-prod.modern.js = 553.67 kB 553.67 kB = 97.38 kB 97.38 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against 0ad7cc5

@gnoff gnoff force-pushed the onheaders-error-handling branch 2 times, most recently from 5b01726 to a5764c4 Compare November 15, 2023 19:44
@gnoff gnoff requested review from sebmarkbage and acdlite November 15, 2023 19:44
`onHeaders` can throw however for now we can assume that headers are optimistic values since the only things we produce for them are preload links. This is a pragmatic decision because React could concievably have headers in the future which were not optimistic and thus non-optional however it is hard to imagine what these headers might be in practice. If we need to change this behavior to be fatal in the future it would be a breaking change.

This commit adds error logging when `onHeaders` throws and ensures the request can continue to render successfully.
@gnoff gnoff force-pushed the onheaders-error-handling branch from a5764c4 to 0ad7cc5 Compare November 15, 2023 20:40
@gnoff gnoff merged commit ee68446 into facebook:main Nov 15, 2023
36 checks passed
@gnoff gnoff deleted the onheaders-error-handling branch November 15, 2023 20:53
github-actions bot pushed a commit that referenced this pull request Nov 15, 2023
`onHeaders` can throw however for now we can assume that headers are
optimistic values since the only things we produce for them are preload
links. This is a pragmatic decision because React could concievably have
headers in the future which were not optimistic and thus non-optional
however it is hard to imagine what these headers might be in practice.
If we need to change this behavior to be fatal in the future it would be
a breaking change.

This commit adds error logging when `onHeaders` throws and ensures the
request can continue to render successfully.

DiffTrain build for [ee68446](ee68446)
kodiakhq bot pushed a commit to vercel/next.js that referenced this pull request Nov 16, 2023
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
`onHeaders` can throw however for now we can assume that headers are
optimistic values since the only things we produce for them are preload
links. This is a pragmatic decision because React could concievably have
headers in the future which were not optimistic and thus non-optional
however it is hard to imagine what these headers might be in practice.
If we need to change this behavior to be fatal in the future it would be
a breaking change.

This commit adds error logging when `onHeaders` throws and ensures the
request can continue to render successfully.
bigfootjon pushed a commit that referenced this pull request Apr 18, 2024
`onHeaders` can throw however for now we can assume that headers are
optimistic values since the only things we produce for them are preload
links. This is a pragmatic decision because React could concievably have
headers in the future which were not optimistic and thus non-optional
however it is hard to imagine what these headers might be in practice.
If we need to change this behavior to be fatal in the future it would be
a breaking change.

This commit adds error logging when `onHeaders` throws and ensures the
request can continue to render successfully.

DiffTrain build for commit ee68446.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants