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

Prevent endless loop when using ref as a callback #256

Closed
wants to merge 1 commit into from

Conversation

jeffsee55
Copy link

Picking up from the advice in this issue #186 I'm noticing an endless loop. Perhaps I'm misusing it but I think we should be able to treat anything called a ref as such.

Sandbox is here - uncomment the code and you should see endless console.log statements.

This PR uses a ref for the intersection object instead, TBH I have no idea why setState on the entry object is causing this and wasn't able to come up with a reliable test to reproduce in Jest. Please advise if there's something I've not considered.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 1293

  • 6 of 6 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.01%) to 98.958%

Totals Coverage Status
Change from base Build 1277: 0.01%
Covered Lines: 107
Relevant Lines: 107

💛 - Coveralls

@thebuilder
Copy link
Owner

thebuilder commented Jul 29, 2019

Hey @jeffsee55

A problem with this PR fix, is that it doesn't update correctly when using multiple thresholds. Since you don't change the boolean value state, but the entry is updated.

Looking at the solution i proposed in #186, I think you create the function with React.useCallback, so it doesn't change between renders.

See here: https://codesandbox.io/embed/inspiring-pond-vyz6f

  const handleRef = React.useCallback(
    node => {
      // Uncomment me to see an endless render in the console
      inViewRef(node);
      ref.current = node;
    },
    [inViewRef]
  );

I think it could make sense to include an FAQ section with this solution. 🤔

@jeffsee55
Copy link
Author

Thanks @thebuilder - that works great for me.

@jeffsee55 jeffsee55 closed this Aug 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants