-
Notifications
You must be signed in to change notification settings - Fork 130
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
Add try/cach to extension bundle #1385
Conversation
Temporarily hack the built javascript bundle for extensions by adding a try/catch around the entire script. This ensures that any unexpected error that may occur gets output to console log and not swallowed by our web worker.
Thanks for your contribution! Depending on what you are working on, you may want to request a review from a Shopify team:
|
Benchmark reportThe following table contains a summary of the startup time for all commands.
|
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.
🚀, having these debug logs would be super helpful. I'm definitely down to merge this as a temporary fix.
@vividviolet can you give us your blessing to ship this as a hotfix to retrieve the error logs? I know you've contributed to CLI a bit in the past and also have very good context on UI Extensions. Ideally, we would need to modify a sandbox worker but as POS UI Extensions are already in Early Access and we've been getting complains from partners about not being able to see errors, this would at least unblock us from a product point of view. |
Can you clarify why this issue happens on POS? Should this not be happening for all extensions if it's related to importing an erroneous script? Also, I'm not sure I'm comfortable with adding a try/catch around the partner's code. This would prevent errors from being caught higher up the chain while loading extensions which we might be already doing. Wouldn't it make more sense for POS or other hosts to do |
@vividviolet, from what I understand, yes, it should be an issue with other platforms too, not only POS. Re: adding @david-mccullars might have more context, he spent quite a bit of time investigating this all previously. |
The proper place would be inside the worker itself before calling importScript. Basically inside the worker.load method. |
It absolutely would. I spent the better part of two days trying to accomplish this, but I could never get the worker code to build. (I'm not even 100% certain I was in the right place.) This PR wasn't really intended to be a real PR but was put together to help @henrytao-me see the issues I was experiencing when an extension fails to load without communicating anything meaningful to end users. However, partners are running into the same issue, and @heltisace was thinking this might provide some relief until we can figure out the long-term solution. We definitely welcome any thoughts or advice! |
@vividviolet would you be able to help us do that? |
Yes, I can. I think you are using the same worker script from web correct? @henrytao-me do you remember? |
@vividviolet, I think we are, I looked into this pretty deep at some point too. I think this is the place that needs to be modified. |
Here is a cdn link to the worker we are using: https://cdn.shopify.com/shopifycloud/web/assets/v1/admin-ui-extensions-host/sandbox-worker.worker.js. I got it from the react-native-argo package, |
Yup, that's the same worker used by web. It's strange that we can't catch errors outside of the worker though, I thought this PR to Remote UI was supposed to let us do that: Shopify/remote-dom#173 Have you tried bumping the remote-ui version? |
@david-mccullars @heltisace: can you try to update to latest version of remote-ui? I agree with @vividviolet, the better place for this would be in the worker in Web. |
I'm checking right now. I think both POS and the UI Extensions package are not up to date so I will update both and see what happens. |
After updating the package on the host (POS), the console now throws the errors in the inspect console as expected. We throw an error before the extension loads. I tried to update the client (UI Extensions) too, but it didn't work. This could have been a local issue though, because I used Next steps:
Thanks everyone! |
I don't think you need to update the client libraries because the endpoint set up comes from the host code inside the worker. It should be good enough to just update POS and the |
That's what we decided to do, yeah. |
Temporarily hack the built javascript bundle for extensions by adding a try/catch around the entire script. This ensures that any unexpected error that may occur gets output to console log and not swallowed by our web worker.
WHY are these changes introduced?
When developing native UI extensions for POS, it is possible to get into a state where the extension fails to load. The Smart Grid tile just says "Error loading extension," but there is no further information available. Trying to use chrome://inspect shows no console error messages whatsoever, leaving a partner with no good avenue to debug.
WHAT is this pull request doing?
This PR is a possible short-term solution that ensures that the extension bundle that we build is wrapped in a try/catch so that any error is properly caught and output to the console. Long-term, we should consider modifying the web worker which calls
importScript
to either do a try/catch or ensure that errors are properly reported.How to test your changes?
One easy way of getting into this state is to create a new POS UI extension and then to make the following change to your src/index.js file:
Measuring impact
How do we know this change was effective? Please choose one:
Checklist
dev
ordeploy
have been reflected in the internal flowchart.