search

callstack / super-app-showcase

Monorepository template for super app development with React Native and Re.Pack
MIT License
382 stars 63 forks source link

Feat: add error boundaries #30

Closed jbroma closed 1 year ago

jbroma commented 1 year ago

Summary

Added basic ErrorBoundary component in mini-apps and host & shell app. Used it in places near React.Suspense.

In some cases the navigation bar is present so there's a chance to go back to the previous screen, but in most cases where we import the whole mini-app the NavBar is hidden, and we end up with no way to recover from the ErrorBoundary fallback screen. To keep the experience smooth we should consider adding a 'Go Back' button to go to the previous screen in case of an error.

There is also some repetition involved with ErrorBoundary being copied over to the mini-apps, however this conforms to the pattern present in the repository. In theory implementation could be different in each of the mini-apps but isn't for the sake of simplicity.

andrewworld commented 1 year ago

@jbroma Maybe it makes sense to move such components to some private npm package and just pass renderErrorScreen prop to render different UI if it needed but also provide the default one to not duplicate the logic. Also it would be nice to add Retry button and try to reload the script.

jbroma commented 1 year ago

@jbroma Maybe it makes sense to move such components to some private npm package and just pass renderErrorScreen prop to render different UI and to not duplicate the logic. Also it would be nice to add Retry button and try to reload the script.

I agree 100%. There could be a common package in the monorepo with components that could be included at build time by every app, showcasing even more ways to share code in the monorepo. WDYT?

How would you go about reloading the script? Right now I think remounting the screen could do the trick, but I'm not sure it will trigger the ScriptManager to redownload the script, I guess I will need to check it out.

andrewworld commented 1 year ago

This is a good idea, but we also have News mini-app repo, so in such case private npm package will be just more general solution.

andrewworld commented 1 year ago

I was thinking about using ScriptManager.shared.loadScript directly, but idk if it will work as expected and re-render the component, and also your approach is much better in terms of components behavior consistency

jbroma commented 1 year ago

This is a good idea, but we also have News mini-app repo, so in such case private npm package will be just more general solution.

I think that's a right approach for the proper app but might be an overkill for the template here. I suppose to limit the scope and keep it simple we could just duplicate the ErrorBoundary in the news mini-app.