facebook / react

The library for web and native user interfaces.
https://react.dev
MIT License
228.59k stars 46.79k forks source link

Why is useEffect hook not activating when a component is reloaded after previously throwing an error? #17219

Closed dcs3spp closed 4 years ago

dcs3spp commented 4 years ago

I am learning React and Redux within a Typescript environment. I have managed to implement a container that dispatches a fetch action and subscribes to corresponding fetch success and error state notifications from a redux store. The source code is listed below:

Container

import React, { useEffect } from 'react';

import { connect } from 'react-redux';
import Grid from '@material-ui/core/Grid';
import { GridSpacing } from '@material-ui/core/Grid';

import Course from '../components/Course/Course';

import { courseModels } from '../redux/features/course';
import { courseSelectors } from '../redux/features/course';
import { fetchCoursesAsync } from '../redux/features/course/actions';
import { RootState } from 'ReduxTypes';

type ErrorReport = { hasError: boolean; error?: Error };
type StateProps = {
  isLoading: boolean;
  courses: courseModels.Course[];
  error: ErrorReport;
};

/**
 * Redux dispatch and state mappings
 */
const dispatchProps = {
  fetchCourses: fetchCoursesAsync.request,
};

const mapStateToProps = (state: RootState): StateProps => ({
  isLoading: state.courses.isLoadingCourses,
  courses: courseSelectors.getReduxCourses(state.courses),
  error: courseSelectors.getReduxCoursesError(state.courses),
});

/**
 * Component property type definitions
 */
type Props = ReturnType<typeof mapStateToProps> & typeof dispatchProps;

/**
 * CourseList component
 */
const CourseList = ({
  courses = [],
  error,
  fetchCourses,
  isLoading,
}: Propas): JSX.Element => {
  // fetch course action on mount
  useEffect(() => {
    console.log('COURSELIST FETCHING COURSES');
    fetchCourses();
  }, [fetchCourses]);

  if (isLoading) {
    return <p>Loading...</p>;
  }

  if (error && error.hasError && error.error) {
    throw error.error;
    // if throw an error then encapsulating error boundary catches and displays.
    // However when the container is loaded again via clicking on a Navbar link the useEffect
    // action does not trigger. 

    // Alternatively, if the error is rendered inside the container then the useEffect hook is 
    // still activated if the container is loaded again (e.g. via clicking on a Navbar link).
    // return <p>{JSON.stringify(error.error, null, 2)}</p>;
  }

  return (
    <div style={{ marginTop: 20, padding: 30 }}>
      {
        <Grid container spacing={2 as GridSpacing} justify="center">
          {courses.map(element => (
            <Grid item key={element.courseID}>
              <Course course={element} />
            </Grid>
          ))}
        </Grid>
      }
    </div>
  );
};

/**
 * Exports
 */
export default connect(
  mapStateToProps,
  dispatchProps,
)(CourseList);

If I throw an error within the container then the encapsulating error boundary catches and displays it. However, when the container is reloaded via clicking on a Navbar link the useEffect action does not trigger. Subsequently, the fetchCourses action is not dispatched.

Why is the useEffect hook not triggered on second load after it previously threw an error?

My ErrorBoundary component includes a home button for navigating to '/'. However, after clicking home, if I then click on link to display my CourseList container the ErrorBoundary is again displayed. I do not see the console log message displayed from useEffect. When navigating back to '/courses' shouldn't this recreate the CourseList container? Is this not happening because the error was thrown in render previously, so the container is being reused?

What is best practice for resetting a component that threw an error for surrounding ErrorBoundary?

wadamek65 commented 4 years ago

Are you sure the component which throws the error gets unmounted when you navigate to '/' ? Consider the following simple example: https://codesandbox.io/s/focused-silence-s7jpn

When you navigate Home, the error throwing component gets unmounted and on subsequent mounts it will throw and log the error again.

If it is not unmounted, then the useEffect has no reason to run again since you aren't changing the function that is passed into it's dependencies.

dcs3spp commented 4 years ago

Hi @wadamek65 ,

Thanks for responding, appreciated :)

I am new react user, learning react. Would you be able to clarify how a component is unmounted?

My error boundary component has componentWillUnmount overridden. It uses history property provided by react-router to navigate back to the home page.

My CourseList is a functional component that purposefully throws the error when when rendering. It throws the error if there is a fetch request error response state on the redux store. The error is not thrown in the useEffect of the functional component. The useEffect function is only used to dispatch a fetch request action onto redux.

I have included the code listing for Error Boundary and routing component below.....

Error Boundary component - Displays Error Dialog and button to navigate home via this.props.history.push

import React, { Component, ErrorInfo, ReactNode } from 'react';

import Button from '@material-ui/core/Button';
import Dialog from '@material-ui/core/Dialog';
import DialogActions from '@material-ui/core/DialogActions';
import DialogContent from '@material-ui/core/DialogContent';
import DialogContentText from '@material-ui/core/DialogContentText';
import DialogTitle from '@material-ui/core/DialogTitle';
import Typography from '@material-ui/core/Typography';

import * as stackTrace from 'stacktrace-js';
import { RouteComponentProps, withRouter } from 'react-router';
import { UnregisterCallback } from 'history';
import Box from '@material-ui/core/Box';

type ErrorBoundaryProps = {};
type ErrorBoundaryState = {
  hasError: boolean;
  error?: Error;
  info?: ErrorInfo;
};

class ErrorBoundary extends Component<
  ErrorBoundaryProps & RouteComponentProps,
  ErrorBoundaryState
> {
  private static homePath = '/';

  private unlisten!: UnregisterCallback;

  constructor(props: ErrorBoundaryProps & RouteComponentProps) {
    super(props);
    this.state = {
      hasError: false,
      error: undefined,
      info: undefined,
    };
    this.goHome = this.goHome.bind(this);
  }

  componentDidMount(): void {
    this.unlisten = this.props.history.listen(() => {
      if (this.state.hasError) {
        this.setState({ hasError: false });
      }
    });
  }

  componentWillUnmount(): void {
    this.unlisten();
  }

  async componentDidCatch(error: Error, info: ErrorInfo): Promise<void> {
    this.setState({
      hasError: true,
      error: APP_CONF.isProd ? await this.normaliseError(error) : error,
      info: info,
    });
  }

  goHome(): void {
    this.props.history.push(ErrorBoundary.homePath);
  }

  render(): JSX.Element | ReactNode {
    if (this.state.hasError) {
      return (
        <Dialog
          open={this.state.hasError}
          onClose={this.goHome}
          aria-labelledby="alert-dialog-title"
          aria-describedby="alert-dialog-description"
          scroll="paper"
        >
          <DialogTitle
            disableTypography
            id="alert-dialog-title"
            style={{ backgroundColor: 'lightseagreen', color: 'cyan' }}
          >
            <Typography variant="h6">
              {this.state.error
                ? this.state.error.message
                : 'error message unavailable'}
            </Typography>
          </DialogTitle>
          <DialogContent>
            <DialogContentText component="div" id="alert-dialog-description">
              <Typography
                variant="caption"
                style={{ color: 'midnightblue' }}
                component="span"
              >
                <Box textAlign="justify" mt={1} style={{ whiteSpace: 'pre' }}>
                  {this.state.error
                    ? this.state.error.stack
                    : 'stack trace unavailable'}
                </Box>
                <Box textAlign="justify" mt={1} style={{ whiteSpace: 'pre' }}>
                  {this.state.info && APP_CONF.mode !== 'production'
                    ? this.state.info.componentStack
                    : 'component stack unavailable in production'}
                  <br />
                </Box>
              </Typography>
            </DialogContentText>
          </DialogContent>
          <DialogActions>
            <Button onClick={this.goHome}>Home</Button>
          </DialogActions>
        </Dialog>
      );
    }

    return this.props.children;
  }

  private async normaliseError(error: Error): Promise<Error> {
    const stackFrames: Array<
      stackTrace.StackFrame
    > = await stackTrace.fromError(error);

    const stack: string = stackFrames.reduce((prev, cur) => {
      return prev.toString() + cur.toString() + '\n';
    }, '');

    return {
      name: error.name,
      message: error.message,
      stack: stack.trim(),
    };
  }
}

export default withRouter(ErrorBoundary);

Component that accepts the router

import React, { Component, Suspense, lazy } from 'react';

import { BrowserRouter, Route, Switch } from 'react-router-dom';
import ErrorPage from '../errors/ErrorPage';
import ErrorBoundary from '../errors/ErrorBoundary';
import { NavBar } from './NavBar/NavBar';

/**
 * Lazy load definitions
 */
const About = lazy(() =>
  import(
    /*
  webpackChunkName: "about-page",
  webpackPrefetch: true
  */ '../views/About/About'
  ),
);

const CourseList = lazy(() =>
  import(
    /*
  webpackChunkName: "course-list",
  webpackPrefetch: true
  */ '../containers/CourseList'
  ),
);

const Home = lazy(() =>
  import(
    /*
webpackChunkName: "home-page",
webpackPrefetch: true
*/ '../views/Home/Home'
  ),
);

type AppProps = {};

export class App extends Component<AppProps, {}> {
  public render(): JSX.Element {
    return (
      <BrowserRouter>
        <div>
          <NavBar />
          <Suspense fallback={<div>LoaderOptionsPlugin...</div>}>
            <Switch>
              <Route exact path="/" component={Home}></Route>
              <Route exact path="/about" component={About}></Route>
              <Route
                exact
                path="/courses"
              >
                <ErrorBoundary>
                  <CourseList />
                </ErrorBoundary>
              </Route>
              <Route exact path="/error" component={ErrorPage}></Route>
            </Switch>
          </Suspense>
        </div>
      </BrowserRouter>
    );
  }
}
wadamek65 commented 4 years ago

A component unmounts when React decides that a component should not be in the resulting tree and is removed completely. Consider this little sandbox https://codesandbox.io/s/cranky-driscoll-xerls . Notice how all the components keep logging to console when they are remounted repeatedly regardless of useEffect deps. This is what I assumed your problem was - the component was still persisting in the tree and was not unmounted at all and that's why the useEffect was not rerunning.

Unfortunately I can't see the problem with your code (Perhaps the suspense with switch and routes is working weirdly?). If you could reproduce your problem in a code sandbox with minimal amount of code I could look into it a little bit more or maybe someone else will be able to help you :)

(Also I don't quite understand why you are listening to history in your ErrorBoundary component, could you please explain? Maybe that's the problem.)

dcs3spp commented 4 years ago

Hi @wadamek65,

Many thanks for responding. Your help is very much appreciated :) Apologies for the delayed response. I have created a project on stackblitz that replicates the problem that I am encountering:

Hope somebody can help.....

wadamek65 commented 4 years ago

Okay, now I see your problem.

The error that PostsList component receives is taken from the redux store. In ErrorBoundary you only reset the error in the components local state, not in the global redux store - which means that the error still persists in the store and is automatically passed to PostsList on mount and thrown instantly (which is then caught by ErrorBoundary). This loops because the error from redux store is never reset.

You need to create a new redux action that resets the error in the state. Once you have the reset error action you can dispatch it in ErrorBoundary in the history listener. If it is resetted, the PostsList component will not receive an error and the useEffect will run properly.

Edit: Another solution would be to throw the error in the same useEffect you do the fetching so you can actually decide if you want to throw it or refetch.

dcs3spp commented 4 years ago

Thanks @wadamek65.

Is the error state not being reset in the error reducer ( src/redux/reducers.js) upon receipt of FETCH_POSTS_REQUEST and FETCH_POSTS_SUCCESS actions?

wadamek65 commented 4 years ago

The FETCH_POSTS_SUCESS and FETCH_POSTS_REQUEST seem to never be called since the useEffect doesn't have a chance to run before the error is thrown.

dcs3spp commented 4 years ago

Yep, the useEffect only runs once for the initial request. Once an error has been thrown it never seems to run again.

Ah I think I understand what your solution is..... My thinking was resetting the error state in error state reducer for SUCCESS and REQUEST action types. So the best practice is to introduce a dedicated reset error action that is triggered from the error boundary boundary when the user closes via clicking on the home link. That way the race condition with useEffect is avoided. Is my understanding correct?

wadamek65 commented 4 years ago

It would be fine but in the way that your component is right now is that the useEffect that should trigger data fetching and error resetting is never run because right below it the error is constantly rethrown. I believe this is not a race condition but simply the useEffect is first called after mount, while everything inside the component body is like a constructor in a class and called immediately.

Imho, you are mixing two ways of error handling. You should either stick to catching and storing the error in redux store or letting the error boundary catch it itself. What I would do is remove the error throwing from PostsList component, and refactor the error boundary to a simple higher order component that does a few things:

Another solution would be to forego saving the error in redux store and simply let the fetch action throw the error freely that would then be caught by error boundary.

Edit: Or you could just display the error in PostsList component and create a button to refetch the data. There are multiple solutions to your problem, I wouldn't call any of them the "right" one. I am also a little rusty on my redux so I might be not providing the most optimal solution :)

dcs3spp commented 4 years ago

Thanks @wadamek65 for your help, patience and suggestions. Many, many thanks. I will have a think through the suggestions. I like the idea of the higher order component so will probably investigate and try out that option and take it from there....

Again thanks for your help and advice. Much appreciated :)