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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update POS render function to use createRoot #2353

Open
wants to merge 3 commits into
base: unstable
Choose a base branch
from

Conversation

js-goupil
Copy link
Contributor

@js-goupil js-goupil commented Sep 17, 2024

Background

Currently all of our teardown functionality does not work. For example, the return method from a useEffect does not work in UI Extensions for POS (and I think on the other surfaces it's also broken?).

Solution

This replaces the deprecated render call with the createRoot function, which returns a root which has a render function, and an unmount function. We are passing the unmount method into the Promise.resolve, which means that it can be used on the host side to properly unmount everything when POS unmounts the UIExtensionContainer.

🎩

You can tophat this on POS with the following POS Branch: https://github.com/Shopify/pos-next-react-native/pull/42289

  1. Pull this branch to your machine.
  2. Run yarn build, and go to the build directory for ui-extensions-react, and find the esm/surfaces/point-of-sale/render.mjs file that gets generated. Copy the contents.
  3. In a POS UI Extension running locally, paste the contents of that into the node_modules/@shopify/ui-extensions-react/build/esm/surfaces/point-of-sale/render.mjs.
  4. Run your UI Extension and implement a useEffect with a tear down function.
  5. Inject your UI Extension into POS on the branch I specified at the top of this tophat section.
  6. Using a webview debugging console, make sure your tear down is getting called!

Checklist

  • I have 🎩'd these changes
  • I have updated relevant documentation

@js-goupil js-goupil marked this pull request as ready for review September 17, 2024 19:27
Copy link
Contributor

We detected some changes in packages/*/package.json or packages/*/src, and there are no updates in the .changeset directory. If the changes are user-facing and should cause a version bump, run yarn changeset to track your changes and include them in the next release CHANGELOG. If you are making simple updates to repo configuration, examples, or documentation, you do not need to add a changeset.

try {
remoteRender(
const remoteRoot = createRoot(root);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not exactly sure what else would be required in this file, but this seems to work properly. Also it would be nice to know if this change is desired elsewhere. @vividviolet you mentioned we could just share this function implementation instead of having it be duplicated everywhere.

@js-goupil
Copy link
Contributor Author

@NathanJolly Testing this on main of POS without any changes as seen in my other draft PR seems to work just fine. However, I don't feel the need to squeeze this in for 2024-10. If anything breaks on POS because of something we didn't test, well it just doesn't feel worth the risk right now. But perhaps while I'm on intermission you guys can ship this once the 2024-10 cut as been made, and then we can implement the appropriate changes on POS.

Copy link
Member

@vividviolet vividviolet left a comment

Choose a reason for hiding this comment

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

I do think we should share this fix with other surface areas to be consistent

@js-goupil js-goupil force-pushed the js-update-pos-render-to-createroot branch from 1625924 to f84072f Compare September 18, 2024 21:28
@js-goupil js-goupil force-pushed the js-update-pos-render-to-createroot branch from f84072f to 547404d Compare September 18, 2024 21:29
@js-goupil
Copy link
Contributor Author

@vividviolet Updated the PR to move the logic into a shared util function. I can update the admin UI Extension with another commit in this PR if you want, or we can wait until you guys are ready down the line?

@vividviolet
Copy link
Member

@vividviolet Updated the PR to move the logic into a shared util function. I can update the admin UI Extension with another commit in this PR if you want, or we can wait until you guys are ready down the line?

Can you please add it for admin as well? I think we do want it for the upcoming Oct release. I'll test the PR for Admin.

@js-goupil
Copy link
Contributor Author

Can you please add it for admin as well? I think we do want it for the upcoming Oct release. I'll test the PR for Admin.

done

@vividviolet

@js-goupil js-goupil force-pushed the js-update-pos-render-to-createroot branch from d7c898e to dccb7f8 Compare September 19, 2024 13:24
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.

4 participants