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

Retain module init errors instead of retrying #20429

Closed
wants to merge 1 commit into from

Conversation

sebmarkbage
Copy link
Collaborator

@sebmarkbage sebmarkbage commented Dec 10, 2020

Since requiring a module doesn't always rethrow the same error as the first time, we save the error so that it rethrows in all cases.

The downside of this is that pause on uncaught exceptions doesn't work anymore for module init errors. We could potentially try the invokeGuardedCallback thing here.

This is not fool proof because if a different module requires the same thing that errored, then it'll still read an uninitialized version but at least the first error hopefully propagated.

Unfortunately this is still not enough because Fast Refresh can grab a reference to the module exports directly. This then registers the already failed export and then rerenders React with the failed export. Something about how this works also makes this a race condition and the order of errors can change as a result.

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Dec 10, 2020
@codesandbox-ci
Copy link

codesandbox-ci bot commented Dec 10, 2020

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 e54e897:

Sandbox Source
React Configuration

@sizebot
Copy link

sizebot commented Dec 10, 2020

Warnings
⚠️ Build job for base commit is still pending: cdfde3a

Size changes (experimental)

Generated by 🚫 dangerJS against e54e897

@sizebot
Copy link

sizebot commented Dec 10, 2020

Details of bundled changes.

Comparing: cdfde3a...e54e897

react-server-dom-relay

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactFlightDOMRelayClient-dev.js +4.8% +5.5% 11.52 KB 12.07 KB 3.38 KB 3.57 KB FB_WWW_DEV
ReactFlightDOMRelayClient-prod.js 🔺+4.9% 🔺+3.7% 5.52 KB 5.79 KB 1.59 KB 1.65 KB FB_WWW_PROD

react-server-native-relay

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactFlightNativeRelayClient-dev.js +4.7% +5.4% 11.52 KB 12.07 KB 3.38 KB 3.56 KB RN_FB_DEV
ReactFlightNativeRelayClient-prod.js 🔺+4.9% 🔺+3.8% 5.52 KB 5.79 KB 1.58 KB 1.64 KB RN_FB_PROD

react-client

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-client-flight.development.js +3.6% +4.2% 14.31 KB 14.83 KB 4.21 KB 4.39 KB NODE_DEV
react-client-flight.production.min.js 🔺+3.5% 🔺+2.5% 3.27 KB 3.38 KB 1.47 KB 1.51 KB NODE_PROD

react-server-dom-webpack

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-server-dom-webpack-writer.browser.development.server.js 0.0% 0.0% 24.71 KB 24.71 KB 6.75 KB 6.76 KB NODE_DEV
react-server-dom-webpack-node-register.js 0.0% +0.3% 3.2 KB 3.2 KB 1.29 KB 1.3 KB NODE_ES2015
react-server-dom-webpack-writer.browser.production.min.server.js 0.0% 0.0% 6.26 KB 6.26 KB 2.62 KB 2.62 KB NODE_PROD
react-server-dom-webpack-node-loader.js 0.0% 0.0% 8.24 KB 8.24 KB 2.75 KB 2.75 KB NODE_ESM
react-server-dom-webpack-plugin.js 0.0% +0.2% 7.85 KB 7.85 KB 2.56 KB 2.57 KB NODE_ES2015
react-server-dom-webpack.development.js +3.0% +3.5% 18.01 KB 18.56 KB 4.94 KB 5.11 KB UMD_DEV
react-server-dom-webpack.production.min.js 🔺+3.0% 🔺+2.3% 3.93 KB 4.05 KB 1.76 KB 1.8 KB UMD_PROD
react-server-dom-webpack.development.js +3.1% +3.5% 16.8 KB 17.32 KB 4.82 KB 4.99 KB NODE_DEV
react-server-dom-webpack.production.min.js 🔺+3.1% 🔺+2.5% 3.73 KB 3.85 KB 1.67 KB 1.71 KB NODE_PROD
react-server-dom-webpack-writer.browser.development.server.js 0.0% 0.0% 26.32 KB 26.32 KB 6.89 KB 6.9 KB UMD_DEV
react-server-dom-webpack-writer.node.development.server.js 0.0% 0.0% 25.65 KB 25.65 KB 7.01 KB 7.01 KB NODE_DEV
react-server-dom-webpack-writer.browser.production.min.server.js 0.0% 🔺+0.1% 6.46 KB 6.46 KB 2.71 KB 2.72 KB UMD_PROD
react-server-dom-webpack-writer.node.production.min.server.js 0.0% 🔺+0.1% 6.45 KB 6.45 KB 2.65 KB 2.65 KB NODE_PROD

Size changes (stable)

Generated by 🚫 dangerJS against e54e897

Since requiring a module doesn't always rethrow the same error as the first
time, we save the error so that it rethrows in all cases.

This is not fool proof because if a different module requires the same
thing that errored, then it'll still read an uninitialized version but
at least the first error hopefully propagated.
@sebmarkbage
Copy link
Collaborator Author

sebmarkbage commented Dec 10, 2020

Another thing I'm not sure about is why this gets logged to the console twice in prod.

@facebook-github-bot
Copy link

Hi @sebmarkbage!

Thank you for your pull request.

We require contributors to sign our Contributor License Agreement, and yours needs attention.

You currently have a record in our system, but the CLA is no longer valid, and will need to be resubmitted.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@sebmarkbage sebmarkbage deleted the branch facebook:master October 20, 2022 20:41
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.

None yet

4 participants