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

test: Add regression test for hooks after error boundaries #20002

Merged
merged 2 commits into from
Oct 16, 2020

Conversation

eps1lon
Copy link
Collaborator

@eps1lon eps1lon commented Oct 12, 2020

Summary

Closes #15301
Closes #15219

The issue itself is fixed starting with 16.9.0. It was fixed in #15387.

Adding it to ./packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js is not sufficient since the test is passing in v16.8.6 with replayFailedUnitOfWorkWithInvokeGuardedCallback = false

Test Plan

  1. Reproduced error with 16.8.6: https://codesandbox.io/s/caught-error-with-sibling-hooks-o13p0
  2. Could not reproduce with 16.9.0: https://codesandbox.io/s/caught-error-with-sibling-hooks-1690-j79yw
  3. Add test file
     /**
      * Copyright (c) Facebook, Inc. and its affiliates.
      *
      * This source code is licensed under the MIT license found in the
      * LICENSE file in the root directory of this source tree.
      *
      * @emails react-core
      */
    
     'use strict';
    
     let React;
     let ReactDOM;
    
     describe('ReactErrorBoundariesHooks', () => {
       beforeEach(() => {
         jest.resetModules();
         ReactDOM = require('react-dom');
         React = require('react');
       });
    
       it('should preserve hook order if errors are caught', () => {
         function ErrorThrower() {
           React.useMemo(() => undefined, []);
           throw new Error('expected');
         }
    
         function StatefulComponent() {
           React.useState(null);
           return ' | stateful';
         }
    
         class ErrorHandler extends React.Component {
           state = {error: null};
    
           componentDidCatch(error) {
             return this.setState({error});
           }
    
           render() {
             if (this.state.error !== null) {
               return <p>Handled error: {this.state.error.message}</p>;
             }
             return this.props.children;
           }
         }
    
         function App(props) {
           return (
             <React.Fragment>
               <ErrorHandler>
                 <ErrorThrower />
               </ErrorHandler>
               <StatefulComponent />
             </React.Fragment>
           );
         }
    
         const container = document.createElement('div');
         ReactDOM.render(<App />, container);
    
         expect(() => {
           ReactDOM.render(<App />, container);
         }).not.toThrow();
       });
     });
  4. create bisect.sh
     #!/bin/bash
     yarn
     ! yarn test ReactErrorBoundariesHooks
     exit $?
$ git merge-base v16.8.6 v16.9.0 # 3e5556043879c9c7b98dd9edfc0e89df0366714b
$ git bisect start --term-new=fixed --term-old=unfixed
$ git checkout v16.9.0
$ git bisect fixed
$ git checkout 3e5556043879c9c7b98dd9edfc0e89df0366714b # merge base of 16.8.6. and 16.9.0
$ git bisect unfixed
$ git bisect run ./bisect.sh
9055e31e5c82d03f0a365c459f7bc79e402dbef5 is the first fixed commit

@codesandbox-ci
Copy link

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

Sandbox Source
React Configuration
caught error with sibling hooks PR
caught error with sibling hooks (16.9.0) PR
React Issue #15219
immutable-moon-lfgfv Issue #15219
wispy-paper-ujvxe Issue #15301

@sizebot
Copy link

sizebot commented Oct 12, 2020

No significant bundle size changes to report.

Size changes (stable)

Generated by 🚫 dangerJS against e5972d6

@sizebot
Copy link

sizebot commented Oct 12, 2020

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against e5972d6

@matthargett
Copy link
Contributor

great regression test! can this test be pushed down to react-reconciler and use the test or noop renderer? that's closer to where the regression was introduced/fixed, and that's where the ReactIncrementalErrorHandling-test currently is.

@eps1lon
Copy link
Collaborator Author

eps1lon commented Oct 12, 2020

can this test be pushed down to react-reconciler and use the test or noop renderer?

I'll take a look over the week. Thanks for the suggestion!

@eps1lon
Copy link
Collaborator Author

eps1lon commented Oct 15, 2020

@matthargett Running

/**
 * Copyright (c) Facebook, Inc. and its affiliates.
 *
 * This source code is licensed under the MIT license found in the
 * LICENSE file in the root directory of this source tree.
 *
 * @emails react-core
 */

'use strict';

let React;
let ReactFeatureFlags;
let ReactNoop;
let Scheduler;

describe('ReactIncrementalErrorReplay', () => {
  beforeEach(() => {
    jest.resetModules();
    ReactFeatureFlags = require('shared/ReactFeatureFlags');
    ReactFeatureFlags.enableNewScheduler = false;
    React = require('react');
    ReactNoop = require('react-noop-renderer');
    Scheduler = require('scheduler');
  });

  it('should preserve hook order if errors are caught', () => {
    function ErrorThrower() {
      React.useMemo(() => undefined, []);
      throw new Error('expected');
    }

    function StatefulComponent() {
      React.useState(null);
      return null;
    }

    class ErrorHandler extends React.Component {
      state = {error: null};

      componentDidCatch(error) {
        return this.setState({error});
      }

      render() {
        if (this.state.error !== null) {
          return <p>Handled error: {this.state.error.message}</p>;
        }
        return this.props.children;
      }
    }

    function App(props) {
      return (
        <React.Fragment>
          <ErrorHandler>
            <ErrorThrower />
          </ErrorHandler>
          <StatefulComponent />
        </React.Fragment>
      );
    }

    ReactNoop.render(<App />);
    Scheduler.flushAll();

    ReactNoop.render(<App />);
    Scheduler.flushAll();
  });
});

right before #15387 (which was 4e59d4f) does not throw. There were some react-dom specific test branches removed (https://github.com/facebook/react/pull/15387/files#diff-67f2b3ded286ca57e398e1d05ad22e024b2dd7f91fb1016538756af542f9b9e1) so react-dom might've been the only renderer affected by this bug.

@gaearon gaearon merged commit b093528 into facebook:master Oct 16, 2020
@gaearon
Copy link
Collaborator

gaearon commented Oct 16, 2020

Thanks!

@eps1lon eps1lon deleted the test/error-boundary-sibling-hooks branch October 16, 2020 16:15
koto pushed a commit to koto/react that referenced this pull request Jun 15, 2021
…20002)

* test: Add regression test for hooks after error boundaries

* fix lint
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants