Skip to content

Add warning about setState within useTracker#320

Closed
edemaine wants to merge 2 commits intometeor:masterfrom
edemaine:patch-2
Closed

Add warning about setState within useTracker#320
edemaine wants to merge 2 commits intometeor:masterfrom
edemaine:patch-2

Conversation

@edemaine
Copy link
Copy Markdown
Contributor

@edemaine edemaine commented Feb 7, 2021

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
Copy link
Copy Markdown
Contributor

CaptainN commented Feb 8, 2021

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
Copy link
Copy Markdown
Contributor Author

@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
Copy link
Copy Markdown
Contributor

@CaptainN could you review one last time?

@filipenevola filipenevola added the waiting-feedback It's implemented but we need feedback that it is working as expected label Feb 10, 2021
@CaptainN
Copy link
Copy Markdown
Contributor

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
Copy link
Copy Markdown
Collaborator

@CaptainN any update on this?

@StorytellerCZ StorytellerCZ requested a review from CaptainN April 30, 2021 14:04
@CaptainN
Copy link
Copy Markdown
Contributor

CaptainN commented Apr 30, 2021

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

@CaptainN
Copy link
Copy Markdown
Contributor

CaptainN commented May 13, 2021

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
Copy link
Copy Markdown
Contributor Author

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

@edemaine edemaine closed this May 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

waiting-feedback It's implemented but we need feedback that it is working as expected

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants