Skip to content
This repository has been archived by the owner on Nov 20, 2020. It is now read-only.

useEffect is called on every re-render when using simple as a dependency #55

Closed
Pet3ris opened this issue Oct 16, 2019 · 4 comments · Fixed by #56
Closed

useEffect is called on every re-render when using simple as a dependency #55

Pet3ris opened this issue Oct 16, 2019 · 4 comments · Fixed by #56

Comments

@Pet3ris
Copy link

Pet3ris commented Oct 16, 2019

Hi There,

I'm facing an issue very similar to this one.

However, my hook is related to the simple argument of the query, not the full argument. It does unfortunately trigger the hook on every re-render.

Any thoughts on why that is if the underlying query value is not changing?

@Qziem
Copy link

Qziem commented Oct 19, 2019

I think that better to use full in useEffect1 argrument. But you can still use simple inside.
Try:

let (simple, full) = UserQuery.use();
React.useEffect1(
  () => {
    Js.log2("useEffect called", simple);
    None;
  },
  [|full|],
);

I tried this and looks like its called only when full is changed, not every rerender. I think its becouse full its record which is immutable structure.

@Qziem
Copy link

Qziem commented Oct 19, 2019

I probably know why is that:
In Query.re calculation full is in React.useMemo which means that it is calculated only when its change. But calulate simple is not in React.useMemo - but in my opinion it should be (like in Mutation.re)

@Pet3ris
Copy link
Author

Pet3ris commented Oct 19, 2019

@kamilkuz - that's AMAZING! 🙌 Thank you - love a simple fix. I will leave it open for now before the authors can verify if it's seen as a bug or not.

@fakenickels
Copy link
Member

Hey there! Sorry about the delay we have been out for a conference in this weekend and will get back here soon.

I agree that simple should be also in a React.useMemo!

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.

3 participants