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

[suspense][error handling] Inline renderRoot and fix error handling bug #16801

Merged
merged 9 commits into from Sep 23, 2019

Conversation

@acdlite
Copy link
Member

@acdlite acdlite commented Sep 17, 2019

Follow-up to work loop refactor in #16743.

After the recent changes, the isSync argument to renderRoot is only used in a single place, to determine whether to call workLoop or workLoopSync. All other forked behavior was lifted into performConcurrentWorkOnRoot and performSyncWorkOnRoot. So, this goes one step further and inlines renderRoot into its callers.

Most of the changes here are copy-pasting and moving stuff around. The high level flow is essentially the same. I split the work into small commits to help with reviewing.

The final commit is unrelated, but I've included it because it fixes the test I added in #16800. The fix relies on the handleError function I extracted as part of this refactor PR. I could also submit a separate fix that's independent of this PR; however, since it's not a super urgent bug, and I was planning to do these refactors regardless, I think it's fine to group these.

Fixes #16800

@sizebot
Copy link

@sizebot sizebot commented Sep 17, 2019

ReactDOM: size: 0.0%, gzip: 0.0%

Details of bundled changes.

Comparing: a87d245...d69643c

react-art

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-art.development.js +0.1% -0.0% 670.85 KB 671.54 KB 145.51 KB 145.51 KB UMD_DEV
react-art.production.min.js 🔺+0.3% 🔺+0.2% 103.57 KB 103.87 KB 31.56 KB 31.62 KB UMD_PROD
react-art.development.js +0.1% -0.0% 601.53 KB 602.17 KB 128.09 KB 128.08 KB NODE_DEV
react-art.production.min.js 🔺+0.5% 🔺+0.2% 68.56 KB 68.88 KB 20.82 KB 20.86 KB NODE_PROD
ReactART-dev.js 0.0% 0.0% 617.49 KB 617.74 KB 127.91 KB 127.95 KB FB_WWW_DEV
ReactART-prod.js 🔺+0.2% -0.0% 232.11 KB 232.64 KB 39.42 KB 39.41 KB FB_WWW_PROD

react-native-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactNativeRenderer-prod.js 🔺+0.2% 🔺+0.1% 275.29 KB 275.94 KB 47.15 KB 47.18 KB RN_OSS_PROD
ReactNativeRenderer-profiling.js +0.3% 0.0% 284.68 KB 285.47 KB 48.86 KB 48.87 KB RN_OSS_PROFILING
ReactFabric-prod.js 🔺+0.2% 🔺+0.1% 267.04 KB 267.69 KB 45.73 KB 45.79 KB RN_OSS_PROD
ReactFabric-profiling.js +0.3% +0.1% 276.86 KB 277.65 KB 47.53 KB 47.58 KB RN_OSS_PROFILING
ReactFabric-dev.js 0.0% 0.0% 756.55 KB 756.8 KB 159.46 KB 159.49 KB RN_FB_DEV
ReactFabric-prod.js 🔺+0.2% 🔺+0.1% 267.05 KB 267.69 KB 45.74 KB 45.8 KB RN_FB_PROD
ReactNativeRenderer-dev.js 0.0% 0.0% 749.95 KB 750.2 KB 158.29 KB 158.33 KB RN_OSS_DEV
ReactFabric-profiling.js +0.3% +0.1% 276.86 KB 277.65 KB 47.54 KB 47.59 KB RN_FB_PROFILING
ReactNativeRenderer-dev.js 0.0% 0.0% 750.11 KB 750.36 KB 158.37 KB 158.41 KB RN_FB_DEV
ReactNativeRenderer-prod.js 🔺+0.2% 🔺+0.1% 275.29 KB 275.93 KB 47.16 KB 47.19 KB RN_FB_PROD
ReactNativeRenderer-profiling.js +0.3% 0.0% 284.67 KB 285.46 KB 48.87 KB 48.88 KB RN_FB_PROFILING
ReactFabric-dev.js 0.0% 0.0% 756.38 KB 756.63 KB 159.38 KB 159.41 KB RN_OSS_DEV

react-reconciler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-reconciler.development.js +0.1% -0.0% 600.22 KB 600.86 KB 126.81 KB 126.78 KB NODE_DEV
react-reconciler.production.min.js 🔺+0.4% 🔺+0.3% 70.65 KB 70.95 KB 20.89 KB 20.96 KB NODE_PROD
react-reconciler-persistent.development.js +0.1% -0.0% 597.23 KB 597.87 KB 125.55 KB 125.52 KB NODE_DEV
react-reconciler-persistent.production.min.js 🔺+0.4% 🔺+0.3% 70.66 KB 70.96 KB 20.89 KB 20.96 KB NODE_PROD

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.profiling.min.js +0.2% +0.2% 117.47 KB 117.71 KB 37 KB 37.08 KB NODE_PROFILING
ReactDOM-dev.js 0.0% 0.0% 962.26 KB 962.52 KB 212.07 KB 212.1 KB FB_WWW_DEV
react-dom-server.browser.production.min.js 0.0% 0.0% 19.86 KB 19.86 KB 7.37 KB 7.37 KB UMD_PROD
react-dom-test-utils.production.min.js 0.0% 0.0% 11.19 KB 11.19 KB 4.16 KB 4.16 KB UMD_PROD
react-dom-test-utils.production.min.js 0.0% -0.0% 10.96 KB 10.96 KB 4.09 KB 4.09 KB NODE_PROD
react-dom-unstable-fizz.browser.production.min.js 0.0% 🔺+0.2% 1.04 KB 1.04 KB 632 B 633 B NODE_PROD
react-dom.development.js +0.1% -0.0% 937.14 KB 937.83 KB 211.39 KB 211.38 KB UMD_DEV
react-dom.production.min.js 🔺+0.2% 0.0% 113.79 KB 114.07 KB 36.67 KB 36.67 KB UMD_PROD
react-dom.profiling.min.js +0.2% +0.1% 117.34 KB 117.59 KB 37.68 KB 37.7 KB UMD_PROFILING
react-dom.development.js +0.1% -0.0% 931.21 KB 931.85 KB 209.83 KB 209.83 KB NODE_DEV
react-dom.production.min.js 🔺+0.3% 🔺+0.3% 113.74 KB 114.04 KB 36 KB 36.11 KB NODE_PROD
ReactDOM-prod.js 🔺+0.2% 0.0% 382.1 KB 382.95 KB 69.77 KB 69.77 KB FB_WWW_PROD
ReactDOM-profiling.js +0.3% 0.0% 388.04 KB 389.04 KB 70.82 KB 70.85 KB FB_WWW_PROFILING
react-dom-unstable-native-dependencies.production.min.js 0.0% 0.0% 10.75 KB 10.75 KB 3.67 KB 3.67 KB UMD_PROD
ReactDOMServer-prod.js 0.0% -0.0% 48.55 KB 48.55 KB 11.13 KB 11.13 KB FB_WWW_PROD
react-dom-unstable-fizz.node.development.js 0.0% +0.1% 3.87 KB 3.87 KB 1.51 KB 1.51 KB NODE_DEV
react-dom-unstable-native-dependencies.production.min.js 0.0% 0.0% 10.49 KB 10.49 KB 3.57 KB 3.57 KB NODE_PROD

react-test-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactTestRenderer-dev.js 0.0% 0.0% 628.43 KB 628.69 KB 130.3 KB 130.35 KB FB_WWW_DEV
react-test-renderer-shallow.production.min.js 0.0% 0.0% 11.45 KB 11.45 KB 3.54 KB 3.54 KB UMD_PROD
react-test-renderer-shallow.production.min.js 0.0% 🔺+0.1% 11.6 KB 11.6 KB 3.64 KB 3.64 KB NODE_PROD
react-test-renderer.development.js +0.1% 0.0% 614.88 KB 615.57 KB 130.87 KB 130.88 KB UMD_DEV
react-test-renderer.production.min.js 🔺+0.4% 🔺+0.1% 70.52 KB 70.8 KB 21.63 KB 21.65 KB UMD_PROD
react-test-renderer.development.js +0.1% -0.0% 610.2 KB 610.84 KB 129.67 KB 129.67 KB NODE_DEV
react-test-renderer.production.min.js 🔺+0.4% 🔺+0.1% 70.2 KB 70.51 KB 21.33 KB 21.35 KB NODE_PROD

Generated by 🚫 dangerJS against d69643c

acdlite added 9 commits Sep 17, 2019
Fixes a bug in the Scheduler profiler where the start time of a delayed
tasks is always 0.
Covers an edge case where an error is thrown inside the complete phase
of a component that is in the return path of a component that suspends.
The second error should also be handled (i.e. able to be captured by
an error boundary.

The test is currently failing because there's a call to
`completeUnitOfWork` inside the main render phase `catch` block. That
call is not itself wrapped in try-catch, so anything that throws is
treated as a fatal/unhandled error.

I believe this bug is only observable if something in the host config
throws; and, only in legacy mode, because in concurrent/batched mode,
`completeUnitOfWork` on fiber that throws follows the "unwind" path
only, not the "complete" path, and the "unwind" path does not call
any host config methods.
I want to get rid of the the `isSync` argument to `renderRoot`, and
instead use separate functions for concurrent and synchronous render.

As a first step, this extracts the push/pop logic that happens before
and after the render phase into helper functions.
Similar to previous commit. Extract error handling logic into
a separate function so it can be reused.
Removes `isSync` argument in favor of separate functions.
Moving this out to avoid an accidental early return, which would
bypass the call to `ensureRootIsScheduled` and freeze the UI.
Inlines `renderRoot` into `performConcurrentWorkOnRoot` and
`performSyncWorkOnRoot`. This lets me remove the `isSync` argument
and also get rid of a redundant try-catch wrapper.
Fatal errors (errors that are not captured by an error boundary) are
currently rethrown from directly inside the render phase's `catch`
block. This is a refactor hazard because the code in this branch has
to mirror the code that happens at the end of the function, when exiting
the render phase in the normal case.

This commit moves the throw to the end, using a new root exit status.
@acdlite acdlite force-pushed the acdlite:refactor-and-fix-failing-test branch from d1d5fef to d69643c Sep 23, 2019
@acdlite acdlite merged commit 013b7ad into facebook:master Sep 23, 2019
13 checks passed
13 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: flow Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: lint_build Your tests passed on CircleCI!
Details
ci/circleci: process_artifacts Your tests passed on CircleCI!
Details
ci/circleci: setup Your tests passed on CircleCI!
Details
ci/circleci: test_build Your tests passed on CircleCI!
Details
ci/circleci: test_build_devtools Your tests passed on CircleCI!
Details
ci/circleci: test_build_prod Your tests passed on CircleCI!
Details
ci/circleci: test_dom_fixtures Your tests passed on CircleCI!
Details
ci/circleci: test_source Your tests passed on CircleCI!
Details
ci/circleci: test_source_persistent Your tests passed on CircleCI!
Details
ci/circleci: test_source_prod Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.