Skip to content

Curious about y'alls thoughts.#3

Open
erlandsona wants to merge 4 commits intocowboyd:mainfrom
erlandsona:main
Open

Curious about y'alls thoughts.#3
erlandsona wants to merge 4 commits intocowboyd:mainfrom
erlandsona:main

Conversation

@erlandsona
Copy link
Copy Markdown

Remove need for wrapping individual Prop's as Values and re-rendering replaces the need for a for loop

Comment thread src/enact.tsx Outdated
Comment on lines +42 to +62
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;
Copy link
Copy Markdown
Author

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.

Comment thread src/enact.tsx
return () => {
destroying.current = destroy();
destroying.current.catch((e) => {
console.error("Cleanup failed:", e);
Copy link
Copy Markdown
Author

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 🤔

Copy link
Copy Markdown
Collaborator

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

Copy link
Copy Markdown
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants