Error handling, callbacks and DOMRange

[jankapunkt]

After I investigated a bit further on #372 and #213 I found that my PR #366 is not a real solution:

• it guarantees that members are destroyed BUT now Templates will have no DOMRange in their onDestroyed callback available, which is basically the same issue as in Blaze.remove() destroys DOM before calling onDestroyed() #372
• the root cause is, that a package I used swallowed up an error from onDestroyed which in the end prevented the DOMRange to be removed
• the package itself still surrounded things in a try/catch and monkey-patch removed the view using Blaze.remove
• however when next time visting the template I get DOMRange must be attached
• the whole issue can be fixed by surrounding callbacks (onCreated, onDestroyed, onRendered) with a try/catch that does not bubble up the error but reports it to the Blaze console:

old:

Blaze._fireCallbacks = function (view, which) {
  Blaze._withCurrentView(view, function () {
    Tracker.nonreactive(function fireCallbacks() {
      var cbs = view._callbacks[which];
      for (var i = 0, N = (cbs && cbs.length); i < N; i++)
        cbs[i] && cbs[i].call(view);
    });
  });
};

new:

Blaze._fireCallbacks = function (view, which) {
  Blaze._withCurrentView(view, function () {
    Tracker.nonreactive(function fireCallbacks() {
      var cbs = view._callbacks[which];
      for (var i = 0, N = (cbs && cbs.length); i < N; i++) {
        const cb = cbs[i];
        if (cb) {
          try {
            cb.call(view);
          } catch (e) {
            Blaze._reportException(e, `expection in callback ${which}`)
          }
        }
      }
    });
  });
};

This together with #374 should be the right fix for issues relateded to #372 and #213

Note: I moved too quick with #366 so I thought we discuss this first, before moving on and creating a PR, since I don't like to add PRs that have to be reverted later on. Any thoughts on this @StorytellerCZ @harryadel

[harryadel]

Hey @jankapunkt, thanks for your work and time.

I'm a bit puzzled with all the back and forth and it helps me to have a specific problem to fix instead of thinking abstractly, so I pulled @distalx reproduction repository, and implemented the two proposed solutions in a locally cloned Blaze package:

https://github.com/meteor/blaze/pull/366/files

if (view._domrange)
    view._domrange.destroyMembers(_skipNodes);

    Blaze._fireCallbacks(view, 'destroyed');

#374

if (! view.isDestroyed) {
      var range = view._domrange;
      range.destroy();
      if (range.attached && ! range.parentRange)
      range.detach();
    }

So, would I be mistaken to assume that the current PR doesn't fix the aforementioned problem? Here's the repo I tested with in case you want to give it a spin too. Or the 'Must be attached' problem is completely indifferent to the #372? as @michaelcbrook seems adamant about adding the change.

[jankapunkt]

@harryadel yes the problem in #213 is not solved by #366 and is not directly related to #372, however the current PR #374 is intended only to fix #372 while #213 is not affected by it.

I currently check out the example repo and see what I can find regarding the issue there.
I know that solving for a concrete example sounds reasonable but I'd like to find the root cause of this error so it won't get caused due to a different use-case.

[jankapunkt]

For better understanding:

The error must be attached in #213 is rather related to jQuery event handling when a sub template is in scope of the parent template. When using ostrio:flow-router-extra the event is fired when already moved to a new route (thus no DOMRange attached anymore), however it should not fire at all. This is rather a scoping issue to me and also I wonder if we can prevent the event to pass through.

The error must be attached I prevented with #366 is raised, when using ostrio:flow-router-extra and move away from a template but the template raises an error in onDestroyed, which just looks like it's related to #213 but in fact it's not. So I think there is still a valid use-case for #366 but the related code may change when implementing a fix for

Now ostrio:flow-router-extra also is a bit problematic here, since as stated in #372 it contains some kind of monkey-patching code:

const _BlazeRemove = function (view) {
  try {
    Blaze.remove(view);
  } catch (_e) {
    try {
      Blaze._destroyView(view);
      view._domrange.destroy();
    } catch (__e) {
      view._domrange.destroy();
    }
  }
};

And this is also called when the yielded Template is destroyed:

    Template.yield.onDestroyed(function () {
      if (self.old.template.view) {
        _BlazeRemove(self.old.template.view);
        self.old.template.view = null;
        self.old.materialized = false;
      }
      self.yield = null;
    });

plus on two other lines:

• https://github.com/veliovgroup/flow-router/blob/master/client/renderer.js#L187
• https://github.com/veliovgroup/flow-router/blob/master/client/renderer.js#L205

In my opinion we need to use a fork of ostrio:flow-router-extra and re-test all issues to get a side-effect free result here.

[harryadel]

The error must be attached I prevented with #366 is raised, when using ostrio:flow-router-extra and move away from a template but the template raises an error in onDestroyed, which just looks like it's related to #213 but in fact it's not. So I think there is still a valid use-case for #366 but the related code may change when implementing a fix for

Huh, that makes sense #366 was not an immediate fix to #213 but basically a solution to a problem you found along the way.

Now ostrio:flow-router-extra also is a bit problematic here, since as stated in #372 it contains some kind of monkey-patching code:

In my opinion we need to use a fork of ostrio:flow-router-extra and re-test all issues to get a side-effect free result here.

I agree with you. Maybe @dr-dimitru can chime in? Though, this insinuates if I removed the monkey patch from ostrio in my test repo, I'm bound to get different results? Let me see.

Some side note, but it's unacceptable to merge in PRs with zero testing as in the case of #366 or #374. A regression test must be added for both.

Finally, to get things moving instead of theorizing further, it may beneficial to merge in #374 (with tests ofc) according to #372 (comment) and release 2.6.1 then come up with 2.6.2 for #213 where we can include additional stuff like #373.

Again, I really appreciate your efforts pushing this through and sorry for dipping on you.

[jankapunkt]

Though, this insinuates if I removed the monkey patch from ostrio in my test repo, I'm bound to get different results? Let me see.

Not necessarily but I think we need to get rid of side-effects before testing to make sure this is not a bug introduced by the monkey-patch AND we can then also create a solution that may makes the monkey-patch obsolete.

Some side note, but it's unacceptable to merge in PRs with zero testing as in the case of #366 or #374. A regression test must be added for #366 along with #374.

I agree partially. It is not easy for people to setup a local Blaze project and link it to their current projects, which is why I though the RC-releases to be a good solution since they will never automatically get update but require explicit adding via meteor add or tweaking in .meteor/packages so only people involved in testing will "touch" the RC

Finally, to get things moving instead of theorizing further, it may beneficial to merge in #374 (with tests ofc) according to #372 (comment) and release #371 then come up with 2.6.2 for #213 where we can include additional stuff like #373.

I also partially agree here. I think we should continue doing RC releases until we got it all tested with some running apps and if it's stable we can make 2.6.1. There are still many apps out there using Blaze and we should be very sure that they will not break due to a new release.

Again, I really appreciate your efforts pushing this through and sorry for dipping on you.

No offense taken here. I think we need to be very careful in this current situation to also keep the good name of Meteor and Blaze up.

[dr-dimitru]

@jankapunkt @harryadel thank you for mentioning me. Sure, I'll run tests on my end and study this thread in detail this week. And update package if necessary to get it compatible with latest Meteor and Blaze releases

[dr-dimitru]

@jankapunkt @harryadel I'm working on update. Closed most of the issues. Can you post here proposed changes or come up with PR?

[harryadel]

Thank you for your work @dr-dimitru. I see you already pushed a new version removing the monkey patch. I've reached out to Jan and we're going to look into it and try push things through in the next couple of days.

[jankapunkt]

This is now covered by release 2.6.1 so I'm closing this.