Better error messages

[scott-w]

When I'm using Marionette, I'll occasionally make errors such as accessing a region that doesn't exist e.g.

var MyView = Marionette.LayoutView.extend({
  regions: {
    main: '.main-hook'
  },

  onRender: function() {
    this.showChildView('layout', someView());
  }
});

var view = new MyView();
view.getChildView('layout');

The exception I get comes from inside Marionette as a Cannot read property currentView of undefined or this.getRegion(..) is undefined. While I can chase the stacktrace back to find out the exact issue, it would be much better if I could get the following error:

LayoutError: 'layout' is not a region on MyView. The available regions are: 'main'

This is just to illustrate my point - if we have the information on why something isn't going to work we should tell the developer to save them time.

[paulfalgout]

Related: https://github.com/marionettejs/backbone.marionette/issues/1864 https://github.com/marionettejs/backbone.marionette/issues/1910#issuecomment-55993640

[scott-w]

@paulfalgout Thanks for the links. Just so I understand it, the policy is "if it's an error, then we should describe it"?

My thinking is around having the error tied to the action that's going to fail so, in this case, the error would be raised on getChildView('layout') (or showChildView('layout', view)), and not at view instantiation/definition. At this point in the code, we know the call will fail (the region is undefined) and I feel we should tell the user exactly why.

[paulfalgout]

Just a note that I closed related #2944 and #3078 to be dealt with here

[JSteunou]

can #1910 be closed too?

[paulfalgout]

The ViewDestroyError is now much easier to give better data as it is pretty much only for region.show so we can also tell what region it is occurring in.

[msholty-fd]

@paulfalgout I tried looking around for what ViewDestroyError is but I couldn't find anything in the code nor the issues for this repo. Could you elaborate on what that is?

I am having issues related to this topic - My project using Marionette has async onRender functions, kinda like this:

class Foo extends Mn.View {
  async onRender() {
    this.showChildView('something', new LoadingView());
    await this.model.fetch();
    this.showChildView('something', new SomethingView());
  }
}

The issue is that if the model.fetch promise is still resolving and the user navigates away, destroying the view, we get some error message like this:

Cannot read property 'show' of undefined

This happens in the showChildView function because the region that would have been rendered into is now destroyed. I'd love to just wrap that call with if (!region) or if (region === undefined). In the very least, I'd like a nice error message like "It looks like you tried to render a region that is undefined". We could also give an error message around "This view is destroyed, which means you can't render a view in a region of this view.".

I'd like to hear your thoughts on this, and if you like it I'd be totally willing to open a PR for this.

[paulfalgout]

ViewDestroyError happens when trying to show a destroyed view in a region. You're experiencing the opposite.. though really it's just that the region you're requesting doesn't exist. possibly an error for showChildView only.. I don't know that getRegion should error..

Would entertain an error, but it might wait to merge until 4.0 is in beta or for 4.x at this point.

In any case your fix here is probably

class Foo extends Mn.View {
  async onRender() {
    this.showChildView('something', new LoadingView());
    await this.model.fetch();
    if (this.isDestroyed()) return;
    this.showChildView('something', new SomethingView());
  }
}

[msholty-fd]

Thanks for the response and suggestion @paulfalgout . I also thought that would be sufficient, but I can't help but think adding an if-statement after each await call is a bit overkill when we could simply check in the showChildView function itself.

I tried making a comparison to React to compare how that framework handles this kind of thing, and the main issue in that comparison is that in React the entire UI tree is completely fleshed out before even attaching to the DOM, while Marionette attaches the root template to the DOM and then calls onRender to attach child regions. Because idiomatic Marionette requires you to manually render child regions, and that can happen anywhere in the app render lifecycle (onRender, after an event or in a promise), it just seems prudent to have showChildView check to see if it's even possible to render still.

Do you think a change to showChildView like this would be something that could be introduced?

showChildView: function showChildView(name, view) {
    var region = this.getRegion(name);

    for (var _len = arguments.length, args = Array(_len > 2 ? _len - 2 : 0), _key = 2; _key < _len; _key++) {
      args[_key - 2] = arguments[_key];
    }
    if (!this.isDestroyed()) {
        return region.show.apply(region, [view].concat(args));
    }

    return null;
  },

[paulfalgout]

getUI errors could improve https://gitter.im/marionettejs/backbone.marionette?at=5cd574ef8fcdb05d47b082ed