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

fix(hooks): flush useEffect hooks with a rerender #216

Closed

Conversation

alexkrolick
Copy link
Collaborator

@alexkrolick alexkrolick commented Nov 5, 2018

Fixes #215

This change just calls ReactDOM.render twice in a row to trigger the effects from the first render. It seems like there are cases with subsequent async actions where this could cause unexpected behavior.

EDIT: The second commit changes the approach:

Instead of rendering the same element again, render somewhere else in the document. The second render target doesn't have to be in the body and it seems to trigger the effect.

  if (flushEffects) {
    const Noop = () => null
    ReactDOM.render(<Noop />, document.createElement('template'))
  }

@vitalyso
Copy link
Contributor

vitalyso commented Nov 5, 2018

👍Cool, what about the case with a timer inside a hook?

@kentcdodds
Copy link
Member

Yeah, I'm afraid this wont quite work for many common cases: https://codesandbox.io/s/vqlm09qyx5

I wonder if there's something we can do with the new scheduler package 🤔

@alexkrolick
Copy link
Collaborator Author

Not sure, I dug around in scheduler and didn't see anything missing from the environment that would cause the effects to fail to queue.

@kentcdodds
Copy link
Member

Ok, I did some digging: https://www.youtube.com/watch?v=JQeB9miT9Wc

Conclusion: this wont be enough. I hope we can come up with a better automatic solution to the problem. If we cannot then we'll have to export a flushEffects function in the returned utilities that renders null to a random node in memory.

It's not a terrible solution, but I'd love to find something more automatic if we can...

I wonder if there's a way for us to hook into the scheduler to know when things are scheduled and just make everything run synchronously... Seems dangerous. I'd rather have an officially supported API from React for this... Thoughts?

@gnapse
Copy link
Member

gnapse commented Nov 5, 2018

I'd say yes to the last part. Can't you reach out to someone from the React core team, privately, over twitter or in this very thread or that of the issue?

Update: seem Dan Abramov already agreed there should be an api to trigger them.

@alexkrolick
Copy link
Collaborator Author

It's great there will be an API, but why don't they work automatically? Tests shouldn't have to know when an effect is supposed to be fired and manually call them, that's super brittle and not how the code runs in prod.

As an additional workaround, could we call another render after fireEvent?

@alexkrolick
Copy link
Collaborator Author

Another hacky solution could be to mock React.useEffect to point at React.useLayoutEffect.

@gnapse
Copy link
Member

gnapse commented Nov 5, 2018

I agree with you. Although, if there's an api that the test library (this library in this case) can call internally, it's not that big of a deal. The worse scenario would be if this library ends up having to take this responsibility to the code using it. That's what I'd try to avoid the most.

@kentcdodds
Copy link
Member

Yeah @alexkrolick, those occurred to me as well. I'm going to reach out to some folks and ask them what they think.

@ovidiuch
Copy link
Contributor

ovidiuch commented Nov 5, 2018

It's great there will be an API, but why don't they work automatically?

Million dollar question.

Surely I'm missing something, but since we're using ReactDOM.render and not a test renderer I don't understand why we need to perform any additional operation compared to the regular dev/prod environment. If the effects are async that's not a problem, react-testing-library tests are async by nature. We always wait for elements to appear, etc.

@alexkrolick alexkrolick added the needs discussion Whether changes are needed is still undecided and additional discussion is needed. label Nov 6, 2018
@kentcdodds
Copy link
Member

I don't think this is the direction we should go. At least not right away. Let's wait for a while before we try to solve this problem with something so opinionated.

@kentcdodds kentcdodds closed this Nov 21, 2018
lucbpz pushed a commit to lucbpz/react-testing-library that referenced this pull request Jul 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs discussion Whether changes are needed is still undecided and additional discussion is needed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants