react-juce icon indicating copy to clipboard operation
react-juce copied to clipboard

Handle missing view types with a runtime error

Open nick-thompson opened this issue 5 years ago • 3 comments

Right now, when a view type is requested that we don't support, we just hit a jassert, which is convenient enough if you're working with blueprint's native code, but in an app like soul where you may only be interested in the javascript side of things, that jassert is unfriendly.

Better would be to push an error into the js environment so that the user can recover with a hot reload.

See: https://github.com/nick-thompson/blueprint/blob/master/blueprint/core/blueprint_ViewManager.cpp#L31 https://github.com/nick-thompson/blueprint/blob/master/blueprint/core/blueprint_ReactApplicationRoot.h#L540 https://duktape.org/api.html#duk_push_error_object

We can throw in the ViewManager method call and catch in the ReactAppRoot lambda, the trick will be representing an Error object in the juce::var. I suspect we can do that by:

struct Error : public juce::DynamicObject {};

Then return juce::var(new Error()), and in the ecmascript engine we can check if the var is object and try to dynamic_cast the object to Error... if it succeeds, go the duk_push_error_object route, if not, go the regular push_object route

nick-thompson avatar Sep 06 '20 16:09 nick-thompson

Hm... realizing we could also just maintain a list of supported view types in the reconciler/backend and if the requested view type is not in that list we can throw right there. Sounds much easier...

nick-thompson avatar Sep 06 '20 21:09 nick-thompson

Better would be to push an error into the js environment so that the user can recover with a hot reload.

Wasn't able to run Soul example itself with current react-juce version possibly due to internal renamings ( blueprint->react-juce ). I think this could be achieved with webpack-dev-server as well as webpack-dev-middleware if I understand correctly the problem. So errors would appear in IDE console / terminal. Of course at the expense of more complicated webpack setup. Is it something worth considering?

ilionic avatar Jan 31 '21 12:01 ilionic

@ilionic mentioned in Discord, but the Soul issue is just that Soul will need to update to the latest react_juce native.

For this issue though, things really start right here: https://github.com/nick-thompson/blueprint/blob/master/react_juce/core/ViewManager.cpp#L64

Right there we have a jassert that seeks to ensure that the view you're trying to create is one that we actually support. Generally the work here, therefore, involves two things:

  • Instead of a jassert here, let's properly throw std::invalid_argument
  • Then comes the question: where and how do we catch that invalid_argument so that we can raise the error inside the javascript environment?

If we can check those two boxes, we already have all of the facilities necessary for handling the error (either using React's error boundaries, or letting the error go uncaught within the js environment, in which case we're already set up to catch it again on the native side and show the red stack trace).

Now, that second bullet is a little tricky, but in fact I think we can just wrap the registered native methods: https://github.com/nick-thompson/blueprint/blob/master/react_juce/core/EcmascriptEngine.cpp#L842. Something like

registerNativeProperty(name, juce::var(juce::var::NativeFunction {
  [this, fn = std::move(fn)](const juce::var::NativeFunctionArgs& args) {
    try
    {
        return std::invoke(fn, args);
    }
    catch (std::exception& e)
    {
        mPimpl->raiseException(e);
    }
  }
}));

What do you think?

nick-thompson avatar Jan 31 '21 16:01 nick-thompson