Remove all useEffect usages
#12659
Remove all useEffect usages
#12659
Conversation
playground/src/main.tsx
Outdated
| </React.StrictMode>, | ||
| ); | ||
| }) | ||
| .catch((error) => console.error("Failed to start playground", error)); |
There was a problem hiding this comment.
Doesn't this mean that nothing on the page will render until after the data fetch?
There was a problem hiding this comment.
Yes, that's correct. But arguably, this is the more "correct" behavior in the sense that showing anything is kind of meaningless and risks the user starting to change the code only so that it gets overridden once the response comes in.
I think the "proper" solution would be to split our skeleton:
- The skeleton renders the header, left, and right navigation. It embeds the editor area
- Use a context to store the source and revision
- Fetch the code in the editor area
- Wrap the editor area in a suspense boundary
But that's a larger refactor than what I want to do right now.
There was a problem hiding this comment.
But, on main, we don't even show the editor pane until the source is loaded, right?
There was a problem hiding this comment.
We did show the editor. See https://github.com/astral-sh/ruff/pull/12659/files#r1704097956
Isn't that changing this is your main concern?
There was a problem hiding this comment.
I thought we showed the chrome (header) but not the editable panes in the editor, just based on reading the code where we have that source ? ternary in the editor body.
There was a problem hiding this comment.
ah yeah, that's correct.
There was a problem hiding this comment.
I defer to you, I don't want to get in your way.
There was a problem hiding this comment.
I extracted a Chrome component. It would have been nice if React.use weren't experimental :(
MichaReiser
added
playground
|
I (or someone) should probably test sharing before we merge this. |
Following [You might not need an effect](https://react.dev/learn/you-might-not-need-an-effect#initializing-the-application), remove `useEffect` to initialize the application and instead run it before calling into React.
MichaReiser
force-pushed
the
reduce-use-effect-usage
branch
from
ef99d78 to
7ff695a
Compare
Same as for the application state. We can run the logic exactly once and this way, avoid the hook.
Not sure why I used an effect here in the first place. The effect can be replaced with `useMemo`
MichaReiser
force-pushed
the
reduce-use-effect-usage
branch
from
7ff695a to
5f01bac
Compare
| ); | ||
| }, [pythonSource, settings]); | ||
|
|
||
| if (initPromise.current == null) { |
There was a problem hiding this comment.
Too bad that React.use is experimental because we could then store the promise and have an inner component that is wrapped in a Suspense boundary and uses the promise. For now, this is a hacky version that doesn't show a loading indicator
playground/src/main.tsx
Outdated
| </React.StrictMode>, | ||
| ); | ||
| }) | ||
| .catch((error) => console.error("Failed to start playground", error)); |
There was a problem hiding this comment.
I extracted a Chrome component. It would have been nice if React.use weren't experimental :(
|
Okay, I had to accept or decline the telemetry option, then force kill all wrangler process to get it working. But sharing works |
Summary
The primary usage of effects is to interact with an external system. We don't have this. That's why this PR replaces all
useEffectusages with better suited hooks or removes them alltogether. See https://react.dev/learn/you-might-not-need-an-effectTest Plan
I wasn't able to test sharing. The wrangler server goes up to 100% CPU usage whenever I hit share with or without my changes...