fix: avoid double-render #68

kentcdodds · 2020-09-11T03:05:15Z

What: fix: avoid double-render

Why: Closes #66 and Closes #67

How:

Remove the setState call in componentDidCatch
Ensure that the cDU only resets if the component is updated after the first update with an error.

Checklist:

Documentation
Tests
Ready to be merged

BREAKING CHANGE: This removes the componentStack in the props given to the FallbackComponent and fallbackRender

BREAKING CHANGE: This removes the `componentStack` in the props given to the `FallbackComponent` and `fallbackRender`

codecov-commenter · 2020-09-11T03:09:37Z

Codecov Report

Merging #68 into master will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master       #68   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines           36        40    +4     
  Branches        11        11           
=========================================
+ Hits            36        40    +4

Impacted Files	Coverage Δ
src/index.js	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 463c62e...13f0ffd. Read the comment docs.

bvaughn

Thanks for tackling this and removing the second render 😁

bvaughn · 2020-09-11T13:27:55Z

src/index.js

    this.setState(initialState)
  }

  componentDidCatch(error, info) {
-    this.props.onError?.(error, info?.componentStack)
-    this.setState({info})


bvaughn · 2020-09-11T13:35:34Z

src/index.js

+    // error to be thrown.
+    // So we make sure that we don't check the resetKeys on the first call
+    // of cDU after the error is set
+    if (error !== null && !this.updatedWithError) {


I don't think I fully understand why the resetKeys feature is helpful, or what the purpose of onResetKeysChange does– but the way you're using the updatedWithError instance attribute looks safe (since it's only modified during the commit phase or during an imperative reset "action".

Yeah, it's actually quite helpful. Here's an example of it in action: https://react-hooks.netlify.app/6 (It's the Extra Credit 8, which you can play with here: https://react-hooks.netlify.app/isolated/final/06.extra-8.js)

Basically, if there's a property that you anticipate resulting in an error to be thrown, then you can put it in the resetKeys array and after an error has been thrown, if any values in the array changes then it will be reset automatically.

Here's a gif that demonstrates what this does. With the reset keys, the error boundary is automatically reset when I change the pokemon name:

Thanks for elaborating. The concept seems a bit odd to me, but not necessarily in a bad way. It could just be because it's a different way of thinking about using the boundary than would have occurred to me.

I guess it's roughly similar to using an id-based key or something to completely reset a subtree between view changes.

Anyway 😄 it doesn't look like it would cause problems. (Instance properties can sources of subtle side effects but I think this one looks fine.)

bvaughn · 2020-09-11T13:36:58Z

src/index.js

      this.props.onResetKeysChange?.(prevProps.resetKeys, resetKeys)
-      this.setState(initialState)
+      this.reset()


Shouldn't this be calling resetErrorBoundary? Otherwise you won't call props.onReset.

props.onReset will be called only for when this.resetErrorBoundary is called (presumably the end-user clicked a button you rendered in your fallback that triggered this). props.onResetKeysChange will be called only for when the reset happened due to a change in the resetKeys. It's possible these will both be set to the same function, but I think having the distinction could be helpful which is why they're different functions.

I'm open to your feedback on this though. Great opportunity to break things since we'll be pushing a major version bump.

Hm. I see what you mean, but I would have assumed (if I were using this component) that onReset was called any time the boundary reset (for any reason) and onResetKeysChange was called any time the reset keys changed and those things are decoupled (because one can happen without the other).

How do you actually observe when a reset happens because of a key change? (Using onResetKeysChange won't be sufficient to tell you this, will it)?

How do you actually observe

Is "you" referring to the developer user or library author? 😅

For the developer, they'll know because onResetKeysChange will be called. The lib author knows because we do this check in cDU to compare the previous resetKeys with the next ones.

I was referring to the library user.

And, right. I forgot that we only check and call onResetKeysChange if the error !== null (because intuitively, I still think keys changing is separate from resetting).

Anyway, maybe this isn't a valuable conversation to continue. It just seemed a little unintuitive to me, but I'm only one person.

It's quite possible this particular feature is a bit niche. But it's definitely solved real problems for me so that's why I think it's worth the added complexity 😅

Thanks for your feedback @bvaughn! Always appreciate your time :)

kentcdodds

Thanks for taking the time to review this and provide feedback.

kentcdodds · 2020-09-11T15:13:17Z

src/index.js

+    // error to be thrown.
+    // So we make sure that we don't check the resetKeys on the first call
+    // of cDU after the error is set
+    if (error !== null && !this.updatedWithError) {


Yeah, it's actually quite helpful. Here's an example of it in action: https://react-hooks.netlify.app/6 (It's the Extra Credit 8, which you can play with here: https://react-hooks.netlify.app/isolated/final/06.extra-8.js)

Basically, if there's a property that you anticipate resulting in an error to be thrown, then you can put it in the resetKeys array and after an error has been thrown, if any values in the array changes then it will be reset automatically.

Here's a gif that demonstrates what this does. With the reset keys, the error boundary is automatically reset when I change the pokemon name:

kentcdodds · 2020-09-11T15:15:25Z

src/index.js

      this.props.onResetKeysChange?.(prevProps.resetKeys, resetKeys)
-      this.setState(initialState)
+      this.reset()


props.onReset will be called only for when this.resetErrorBoundary is called (presumably the end-user clicked a button you rendered in your fallback that triggered this). props.onResetKeysChange will be called only for when the reset happened due to a change in the resetKeys. It's possible these will both be set to the same function, but I think having the distinction could be helpful which is why they're different functions.

I'm open to your feedback on this though. Great opportunity to break things since we'll be pushing a major version bump.

kentcdodds · 2020-09-11T17:10:50Z

🎉 This PR is included in version 3.0.0 🎉

The release is available on:

npm package (@latest dist-tag)
GitHub release

Your semantic-release bot 📦🚀

ranisalt · 2020-09-19T22:26:37Z

Great, now how do I get the component stack? It was essential for me.

kentcdodds · 2020-09-19T22:37:02Z

Use the old version or use the onError handler. Or you could write your own error boundary.

bvaughn · 2020-09-19T23:07:51Z

@ranisalt This component is very small. If you have a use case it doesn't solve- fork it.

ranisalt · 2020-09-20T02:11:49Z

I see. A combination of onError and useState solves this. Thanks 👍

I noticed that when using `react-error-boundary@2` (without bvaughn/react-error-boundary#68), and fixing my oversights from the error boundary PR, they work properly with the intended `resetKeys` prop instead of `key`, and we thus avoid the tree unmount issue that `key` introduced.

karlhorky · 2021-07-04T15:02:02Z

@ranisalt how exactly did you do this? Is it similar to the code below from this gist?

export default function MyApp({ Component, pageProps }: { Component: any; pageProps: any }) {
  // ReactErrorBoundary doesn't pass in the component stack trace.
  // Capture that ourselves to pass down via render props
  const [errorInfo, setErrorInfo] = useState<React.ErrorInfo | null>(null);

  return (
    <React.Fragment>
      <ErrorBoundary
        onError={(error, info) => {
          if (process.env.NODE_ENV === 'production') {
            uploadErrorDetails(error, info);
          }
          setErrorInfo(info);
        }}
        fallbackRender={fallbackProps => {
          return <AppErrorFallback {...fallbackProps} errorInfo={errorInfo} />;
        }}
      >
        <Component {...pageProps} />;
      </ErrorBoundary>
    </React.Fragment>
  );
}

karlhorky · 2021-07-04T15:02:46Z

@bvaughn @kentcdodds would you be open to having an example like this in the readme for those who want the component stack trace?

Maybe with the caveat that it would cause a double render?

kentcdodds · 2021-07-04T16:25:47Z

That's fine with me. Most of the time the component stack trace is unnecessary, so make a note about that as well 👍

king-of-poppk · 2023-08-09T10:21:03Z

@ranisalt how exactly did you do this? Is it similar to the code below from this gist?

export default function MyApp({ Component, pageProps }: { Component: any; pageProps: any }) {
  // ReactErrorBoundary doesn't pass in the component stack trace.
  // Capture that ourselves to pass down via render props
  const [errorInfo, setErrorInfo] = useState<React.ErrorInfo | null>(null);

  return (
    <React.Fragment>
      <ErrorBoundary
        onError={(error, info) => {
          if (process.env.NODE_ENV === 'production') {
            uploadErrorDetails(error, info);
          }
          setErrorInfo(info);
        }}
        fallbackRender={fallbackProps => {
          return <AppErrorFallback {...fallbackProps} errorInfo={errorInfo} />;
        }}
      >
        <Component {...pageProps} />;
      </ErrorBoundary>
    </React.Fragment>
  );
}

This commits twice on error right? Once with the current error and the previous info (or null the first time) and once with the current error and info.

@bvaughn, @kentcdodds Will this potentially render a transient invalid state?

fix: avoid double-render

13f0ffd

BREAKING CHANGE: This removes the `componentStack` in the props given to the `FallbackComponent` and `fallbackRender`

kentcdodds requested a review from bvaughn September 11, 2020 03:05

bvaughn reviewed Sep 11, 2020

View reviewed changes

kentcdodds commented Sep 11, 2020

View reviewed changes

kentcdodds merged commit b2f82ce into master Sep 11, 2020

kentcdodds deleted the pr/avoid-double-render branch September 11, 2020 17:07

kentcdodds added the released label Sep 11, 2020

kentcdodds mentioned this pull request Oct 1, 2020

Forward error info to the FallbackComponent #71

Closed

jeysal mentioned this pull request Oct 21, 2020

fix: search field losing focus on key press yldio/asap-hub#383

Merged

jeysal mentioned this pull request Oct 21, 2020

Bug: onResetKeys only triggers reset after second render #74

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: avoid double-render #68

fix: avoid double-render #68

kentcdodds commented Sep 11, 2020

codecov-commenter commented Sep 11, 2020

bvaughn left a comment

bvaughn Sep 11, 2020

bvaughn Sep 11, 2020

kentcdodds Sep 11, 2020

bvaughn Sep 11, 2020

bvaughn Sep 11, 2020

kentcdodds Sep 11, 2020

bvaughn Sep 11, 2020

kentcdodds Sep 11, 2020

bvaughn Sep 11, 2020

kentcdodds Sep 11, 2020

kentcdodds left a comment

kentcdodds Sep 11, 2020

kentcdodds Sep 11, 2020

kentcdodds commented Sep 11, 2020

ranisalt commented Sep 19, 2020

kentcdodds commented Sep 19, 2020

bvaughn commented Sep 19, 2020

ranisalt commented Sep 20, 2020

karlhorky commented Jul 4, 2021

karlhorky commented Jul 4, 2021 •

edited

kentcdodds commented Jul 4, 2021

king-of-poppk commented Aug 9, 2023 •

edited

fix: avoid double-render #68

fix: avoid double-render #68

Conversation

kentcdodds commented Sep 11, 2020

codecov-commenter commented Sep 11, 2020

Codecov Report

bvaughn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kentcdodds left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kentcdodds commented Sep 11, 2020

ranisalt commented Sep 19, 2020

kentcdodds commented Sep 19, 2020

bvaughn commented Sep 19, 2020

ranisalt commented Sep 20, 2020

karlhorky commented Jul 4, 2021

karlhorky commented Jul 4, 2021 • edited

kentcdodds commented Jul 4, 2021

king-of-poppk commented Aug 9, 2023 • edited

karlhorky commented Jul 4, 2021 •

edited

king-of-poppk commented Aug 9, 2023 •

edited