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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds support for async react renders for admin extensions #1678

Merged
merged 1 commit into from
Jan 26, 2024

Conversation

thomas-marcucci
Copy link
Contributor

@thomas-marcucci thomas-marcucci commented Jan 24, 2024

Background

As per this comment in https://github.com/Shopify/ui-api-design/issues/227 we want to lift where an extension can fetch data to the extension callback

Solution

This solution mirrors the implementation from checkout .

馃帺

You can try out my spin instance which has both async and sync rendering react and js extensions, lemme know if my dev server isn't running.
https://admin.web.arbok.thomas-marcucci.us.spin.dev/store/development-store-1/products/2

Otherwise if you want to test it on your own:
spin up checkout-ui-extension-dev
git clone git@github.com:Shopify/ui-extensions.git
cd into ui-extensions/packages/ui-extensions and yarn link
cd into ui-extensions/packages/ui-extensions-react and yarn link
cd into ui-extensions root and yarn build

create an app and generate an admin block extension
cd into your extension's folder and yarn link @shopify/ui-extensions and yarn link @shopify/ui-extensions-react
change the render function in your extension to be be async

export default reactExtension(TARGET, async (api) => {
  return <App />;
});

Now yarn dev and open up the extension preview. It should work as normal. If you throw an exception it should show in the extension's place. If you throw an exception in the render, it should be swallowed and the extension will render its default version (extension name as title and empty content).

Checklist

  • I have 馃帺'd these changes
  • I have updated relevant documentation

@thomas-marcucci thomas-marcucci marked this pull request as ready for review January 25, 2024 14:44

This comment has been minimized.

| ((
api: ApiForRenderExtension<ExtensionTarget>,
) => Promise<ReactElement<any>>)
| ((api: ApiForRenderExtension<ExtensionTarget>) => ReactElement<any>),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of a union of function types, can we change the return type to be a union (ReactElement<any> | Promise<ReactElement<any>>?

{render(api as ApiForRenderExtension<ExtensionTarget>)}
</ExtensionApiContext.Provider>,
root,
() => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Checkout's version of this registration function (https://github.com/Shopify/ui-extensions/blob/unstable/packages/ui-extensions-react/src/surfaces/checkout/render.tsx#L44-L62), we kept the new Promise wrapper, so that we could resolve the promise only in the post-rendering callback React gives us. I think we should match that implementation here.

@thomas-marcucci thomas-marcucci force-pushed the async-react-render-support branch 2 times, most recently from 2f512c4 to 7919228 Compare January 25, 2024 18:34
Copy link
Contributor

@cpeddecord cpeddecord left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

馃帺'ed with your extension code and some validation code as well, code matches checkout's implementation less the error boundary, not sure if that's needed now or if we should follow up.

return extension<'Playground'>(target as any, async (root, api) => {
const element = await render(api as ApiForRenderExtension<ExtensionTarget>);

return new Promise<void>((resolve, reject) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checkout awaits this, which I personally like a little better. But it ends up working out to the same thing for the caller of this function, so it's up to you if you'd prefer to match or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! I didn't intend to leave it with the return. I was playing around with it and didn't catch it when I was putting it back

@thomas-marcucci thomas-marcucci force-pushed the async-react-render-support branch 2 times, most recently from 0fecfb2 to 800152f Compare January 26, 2024 16:14
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: is this really a minor change or a patch? I would argue that its more of a small change (patch) to an existing feature of rendering extensions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right! The tool made me think I had to select major or minor lol

@thomas-marcucci thomas-marcucci merged commit bdcc31c into unstable Jan 26, 2024
5 checks passed
@thomas-marcucci thomas-marcucci deleted the async-react-render-support branch January 26, 2024 18:38
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.

None yet

4 participants