Add deps to react useEffect calls in hackernews example

[jberdine]

No description provided.

[jberdine]

This is what I mentioned in the meeting where our example code seems to be unidiomatic. What do others think?

[jberdine]

Here, the linter wants

Suggested change

	[],
	[skipclient],

[jberdine]

In this case it is not clear to me that it is ok to execute the body of the useEffect more than once. Here the proposed dep skipclient is also constant, so maybe it doesn't matter. I am unsure, are others confident?

[bennostein]

Same as for the other comment thread -- I think the linter is right that skipclient is not necessarily constant in general, as it comes from React's useContext. And if that Skip Client context changes, then it would be right to reevaluate this effect, so I think the linter-proposed dependency array looks right. But confirmation from Daniel would be good :)

[jberdine]

Here, the linter wants

Suggested change

	[],
	[post_id],

[jberdine]

For this case, my understanding of react (which could be wrong!) is that adding the post_id dep here would have the effect of triggering reevaluation sooner than the timeout in case post_id changes. But, and maybe I'm confused, isn't post_id constant, and so it makes no difference? Either way, it seems ok to me to add the dep here and appease the linter.

[bennostein]

That's my understanding of react/useEffect semantics here as well. But, post_id comes from useParams, which is React's way of accessing URL parameters and thus not necessarily constant -- so I think it's right to specify it as a dependency here.

The code looks similar enough to this example from the react docs that I suspect that the empty deps array is a relic of some earlier version that was set up that same way.

[jberdine]

Ok thanks, I will change this. I am fundamentally confused by react. What I expect is that at the point that useEffect is called, the function argument gets evaluated to a closure that binds post_id, which has type number and is already evaluated. So I don't see how react can treat it as anything other than a constant number value.

[bennostein]

Within a single render, yes, post_id is a constant number. But if the params change, that will trigger a re-render with some new post_id!

[bennostein]

LGTM -- the linter-preferred version seems better to me, unless @skiplabsdaniel sees some problem