-
Notifications
You must be signed in to change notification settings - Fork 238
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
fix(emotion): fix render loop #78
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
8782b57
to
c5545c7
Compare
@MichaelDeBoey Thanks for the quick response! I looked at this a bit more and I think I found a more correct solution that works with top level error and catch boundaries as well. |
Since this is an example for end users to copy, it would be helpful to add the correct eslint annotation to suppress the "exhaustive dependencies" warning for that one line. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no permissions here, but I just want to confirm that this change works for me. I would love to see this applied since the example is misleading/broken as it stands!
Agreed, this could probably be improved further, but it works, and the example is currently broken, so it seems like a clear win. @chaance can you find time to take a look at this? |
@RobinClowers Thanks for this! Funny to bump into each other again in such a random place :D |
@jmborden 🥂 sure thing! |
I've pushed up the ref change so the relink won't happen twice with strictmode in dev and got rid of the lint disable. I think this should be merged, since it fixes the issue, if we want to update to react 18, that could be a separate PR. |
@chaance anything preventing this from merging? |
This PR has been automatically marked stale because we haven't received a response from the original author in a while 🙈. This automation helps keep the issue tracker clean from issues that are not actionable. Please reach out if you have more information for us or you think this issue shouldn't be closed! 🙂 If you don't do so within 7 days, this PR will be automatically closed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Be sure to run ESLint/Prettier on your files
Even in strict mode, plus we don't have to disable the lint warning.
0f3f5e0
to
e92a512
Compare
Sorry about that, I committed your suggestions through github. I just fixed this and squashed the fix into the previous commit. Thanks for looking at this! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome!
Thanks for taking the time to update this example @RobinClowers! 🙏
If you would like to also create a PR to update this one to React 18, we'd be happy to accept a PR for that as well
This solves the render loop when loading the emotion example. As far as I can see, we only need to re-inject the styles on the client if the top level component gets unmounted (because that removes the document head, where the styles are).
My original PR had removed all of the code related to this re-injection, but we do need it for that case.
Original description below.
However, when you trigger the catch boundary on the client, you loose all the styles, since the entire document is re-rendered. This same problem exists in the Chakra example, but you can't easily tell, since there is no link you can follow to trigger the error boundary on the client.I finally understand what the code I was removed was trying to do (add the styles back in after re-mounting the document), but it was broken.
I'm not sure what the correct fix is, but I think this is better than the broken example we have now. I will try to spend some more time on this soon, to see if I can find a complete solution.Fixes: #1