-
Notifications
You must be signed in to change notification settings - Fork 3
Curious about y'alls thoughts. #3
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
base: main
Are you sure you want to change the base?
Conversation
… replaces the need for a `for` loop
src/enact.tsx
Outdated
| const [scope, destroy] = createScope(); | ||
| scope.set(RenderContext, setContent); | ||
| scope.run(function* () { | ||
| try { | ||
| let result = yield* component(props); | ||
| if (result) { | ||
| setContent(result); | ||
| } | ||
| } catch (e) { | ||
| let error = e as Error; | ||
| setContent( | ||
| <> | ||
| <h1>Component Crash</h1> | ||
| <h3>{error?.message}</h3> | ||
| <pre>{error?.stack}</pre> | ||
| </>, | ||
| ); | ||
| // Block subsequent renders until cleanup function has run to completion. | ||
| if (destroying.current) { | ||
| yield* destroying.current; | ||
| } | ||
| const val = yield* component(props); | ||
| if (val !== undefined) { | ||
| setContent(val); | ||
| } | ||
| }); | ||
| return () => { destroy() }; | ||
| }, []); | ||
| return () => { | ||
| destroying.current = destroy(); | ||
| destroying.current.catch((e) => { | ||
| console.error("Cleanup failed:", e); | ||
| }); | ||
| }; | ||
| }, [props]); | ||
|
|
||
| return content; | ||
| return content.current; |
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.
See I removed the catch block here.
…e React component.
| return () => { | ||
| destroying.current = destroy(); | ||
| destroying.current.catch((e) => { | ||
| console.error("Cleanup failed:", e); |
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.
Should this get re-thrown too 🤔
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.
Why should next render block on previous render clean-up. What would happen between that?
For example, a user intends to type "effection" in a search box but they're "effect" through, should the UI display the result for "effect" and wait until the clean-up for that finishes? Which will give the perception of a frozen app
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.
Why should next render block on previous render clean-up.
@cowboyd is better equipped to answer this that was his expertise...
I think he said that there could be races between renders if subsequent renders didn't wait for cleanups?
Definitely confused my intuitions but those aren't particularly oriented around the effection interface just yet so I'm suspending my own disbelief for now.
Remove need for wrapping individual Prop's as Values and re-rendering replaces the need for a
forloop