search

meteor / react-packages

Meteor packages for a great React developer experience
http://guide.meteor.com/react.html
Other
574 stars 159 forks source link

Add warning about setState within useTracker #320

Closed edemaine closed 3 years ago

edemaine commented 3 years ago

One approach to resolving #317. Feel free to suggest rewording, or rewrite it yourself. It's a bit of a tricky issue to write...

I hesitate to say "don't mix React and Meteor state", because it is fine to set Meteor state (e.g. Session.set) within a React hook (e.g. useEffect). It's just the other way around that seems problematic.

CaptainN commented 3 years ago

That's great! A suggestions to return the state from useTracker, and deal with mutations to that state in the render body, may be called for here as well.

edemaine commented 3 years ago

@CaptainN Just took a stab at this. React recommends/requires making state changes only within effects, so instead of "render body" I mentioned useLayoutEffect (which is perhaps more consistent with useTracker) and useEffect.

filipenevola commented 3 years ago

@CaptainN could you review one last time?

CaptainN commented 3 years ago

It's not exactly right - I'll try to make some more specific suggestions in the coming weeks. The main issue is mixing state managers. It's not really about rerenders (useTracker handles rerenders just fine).

StorytellerCZ commented 3 years ago

@CaptainN any update on this?

CaptainN commented 3 years ago

I'll review this soon. I did add a note to the readme which is related to this in #321

CaptainN commented 3 years ago

I did add a note encouraging users to return data, instead of setting state. But I also fixed an issue where calling a state setter in useTracker during the first run (causing an immediate re-render) will not break the hook. There was a bug there, and its fixed now, so there's no longer an issue with doing it.

I think that might be enough to close this. What do you think, @edemaine?

edemaine commented 3 years ago

Wow, if you fixed the bug, that's an even better resolution to #317! Thanks!