Skip to content
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

Refactor blocks away from useEffect where it makes sense #48225

Open
tomdevisser opened this issue Feb 18, 2023 · 1 comment
Open

Refactor blocks away from useEffect where it makes sense #48225

tomdevisser opened this issue Feb 18, 2023 · 1 comment
Labels
[Feature] Blocks Overall functionality of blocks Needs Technical Feedback Needs testing from a developer perspective. [Type] Code Quality Issues or PRs that relate to code quality

Comments

@tomdevisser
Copy link
Member

What problem does this address?

I just watched this video on the misuses of useEffect and it had some really interesting takes on why useEffect is used for a lot of things for which it shouldn't be used. Something that becomes extra clear since React 18 where it renders twice.

What is your proposed solution?

I'm not a React expert by any means, but I was looking through the core blocks and found a lot of places where useEffect is used for purposes that can be refactored quite easily, especially often when just updating the state. I would love to discuss this with some more advanced React devs here and see if there are any objections against refactoring these before I dive into the code and do the refactors, but I would love to help with this.

Any thoughts?

@gziolo gziolo added [Feature] Blocks Overall functionality of blocks [Type] Code Quality Issues or PRs that relate to code quality Needs Technical Feedback Needs testing from a developer perspective. labels Feb 19, 2023
@kevin940726
Copy link
Member

Thanks for the thoughtful issue! I agree that there are multiple misuses of useEffect in this project that we should improve on. I also tried to fix some of them, but they are not trivial to fix in a fully backward-compatible way. I think the solutions are different case-by-case and require experienced React devs to create those patches and review them. I don't believe it has a high priority (though I wish it has) right now, but I'm more than happy if anyone wants to chime in and make their hands dirty! I'm willing to provide reviews or guidance for such PRs ❤️ . For now, a tracking issue might help, then we can divide the efforts into different PRs to deep-dive into the problems.

FWIW, it's not only a "tech debt" or a code quality issue, it's also causing some UX quirks like the infamous "undo traps". That's why I think it should have a higher priority once we recognize the root cause and the importance of the issue. For now, the benefits of such refactors are still a bit unclear. Perhaps some initial PRs could help us kick off the process and draw some conclusions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks Needs Technical Feedback Needs testing from a developer perspective. [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

No branches or pull requests

3 participants