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

Stateless component rendering twice. Jest. Enzyme #6673

Closed
csantos1113 opened this issue Mar 28, 2019 · 4 comments · Fixed by #6674
Closed

Stateless component rendering twice. Jest. Enzyme #6673

csantos1113 opened this issue Mar 28, 2019 · 4 comments · Fixed by #6674

Comments

@csantos1113
Copy link

csantos1113 commented Mar 28, 2019

Version

react-router@4.3.1

Test Case

I wanted to test a simple stateless component that is returning <Redirect> when a condition is met.
See the code example:
https://codesandbox.io/s/18or04lkp7

  • In the pure-component.test.js test the output is the expected.
  • In the stateless-component.test.js the component is rendered twice and a Redirect warning appears: "You tried to redirect to the same route you're currently on:"

Steps to reproduce

  • Open the codesandbox "Tests" tab
  • Execute the tests
  • Open the codesandbox "Console" tab
  • See the console output

Expected Behavior

  • Stateless component to be rendered once.

Actual Behavior

  • Stateless component is rendered twice and triggers a Redirect warning
    Screen Shot 2019-03-28 at 4 45 16 PM
@csantos1113
Copy link
Author

Here is the same example running with the latest react-router version (5.0.0)
https://codesandbox.io/s/woxn22omy8

And the result is even worst, because the tests do not run at all:

Maximum update depth exceeded. This can happen when a component repeatedly calls setState inside componentWillUpdate or componentDidUpdate. React limits the number of nested updates to prevent infinite loops.

Invariant Violation: Maximum update depth exceeded. This can happen when a component repeatedly calls setState inside componentWillUpdate or componentDidUpdate. React limits the number of nested updates to prevent infinite loops.
    at invariant (https://woxn22omy8.codesandbox.io/node_modules/fbjs/lib/invariant.js:42:15)
    at scheduleWorkImpl (https://woxn22omy8.codesandbox.io/node_modules/react-dom/cjs/react-dom.development.js:12125:13)
    at scheduleWork (https://woxn22omy8.codesandbox.io/node_modules/react-dom/cjs/react-dom.development.js:12082:12)
    at scheduleCapture (https://woxn22omy8.codesandbox.io/node_modules/react-dom/cjs/react-dom.development.js:11958:5)
    at dispatch (https://woxn22omy8.codesandbox.io/node_modules/react-dom/cjs/react-dom.development.js:11979:11)

@csantos1113 csantos1113 changed the title Maximum update depth exceeded. Jest. Enzyme Stateless component rendering twice. Jest. Enzyme Mar 28, 2019
@StringEpsilon
Copy link
Contributor

StringEpsilon commented Mar 28, 2019

That's quite a nasty bug.

The problem is that React doesn't have a way to figure out whether or not it should re-render functional components on updates. So it always re-renders them. (That's why React 16.6 introduced React.memo)

In short:

  1. <Router > renders
  2. Stateless() renders
  3. Redirect() renders
  4. Redirect() pushes onto History (onMount)
  5. <Router > calls setState because of the listener on the history.
  6. Back to step 1.

Simply making Redirect a class component fixes the issue. I'll create a PR soon.

I have to revise that. Making redirect a class component isn't enough. :/

@timdorr
Copy link
Member

timdorr commented Mar 29, 2019

This was previously addressed in #5003 and fixed by #5162. I'm logging off for the evening right now, but that's roughly what's going on here.

I'd argue locationsAreEqual is the problem here. Should key be considered when comparing two locations?

timdorr pushed a commit that referenced this issue Apr 2, 2019
* Redirect: Failing test for rendering in functional component

* Properly check if location matches in <Redirect/>

Fixes #6673

* Redirect: Handle all eventualities of to strings.

* Redirect: Add update depth test with 'to' as location

* Redirect: Simplify and rename tests.

* Redirect: work around locationsAreEquals quirk.
@banyan

This comment has been minimized.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants