-
Notifications
You must be signed in to change notification settings - Fork 35
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
Allow React extensions to return an element asynchronously #823
Conversation
This comment has been minimized.
This comment has been minimized.
return extension<'Playground'>(target as any, (root, api) => { | ||
return new Promise((resolve, reject) => { | ||
return extension<'Playground'>(target as any, async (root, api) => { | ||
const element = await render(api as ApiForRenderExtension<ExtensionPoint>); |
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.
Shouldn't that be in the try/catch
? If render
throws we wouldn't reach the console.error, would we?
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.
Good call, I will move the try/catch around the whole function body.
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.
err actually @marvinhagemeister do you know if the console.error
is even needed anymore? I feel like we should be doing that in the sandbox instead.
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.
Instead of adding to the worker should the Host actually catch the error before calling render/load
and handle them accordingly? I think it worked for POS when they were trying to catch errors importing the script (see Shopify/cli#1385 (comment)).
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.
@vividviolet yeah, I think it would be better if we removed the log here and just keep the reject()
, which is needed since the error can come from the new Promise()
callback. Then, the host sandbox can catch this error and do whatever it likes. Just want to wait for confirmation from Marvin about whether this log is still needed to avoid errors printing weirdly in React.
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.
Looks great - can we please update the main README for @shopify/ui-extensions-react
with the example for fetching data async?
Should we update the vanilla JS version to just do await renderResults
to match or is there a reason why we need to explicitly check for a promise?
return extension<'Playground'>(target as any, (root, api) => { | ||
return new Promise((resolve, reject) => { | ||
return extension<'Playground'>(target as any, async (root, api) => { | ||
const element = await render(api as ApiForRenderExtension<ExtensionPoint>); |
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.
Instead of adding to the worker should the Host actually catch the error before calling render/load
and handle them accordingly? I think it worked for POS when they were trying to catch errors importing the script (see Shopify/cli#1385 (comment)).
Background
This PR expands the ability for an extension to return its initial UI asynchronously by bringing it to the React library. Now, when a function registers an async function that returns a React element, we will wait for that promise to resolve before mounting the resulting element. The example below shows how a developer could use this to indicate that their extension is not ready to render, until a direct API access call is resolved:
Checklist