Blaze.remove() destroys DOM before calling onDestroyed()

[michaelcbrook]

Hi,

This problem only happens when you explicitly call Blaze.remove(view), but this piece of code causes the DOM to be destroyed before calling the onDestroyed callback.

blaze/packages/blaze/view.js

Lines 695 to 697 in a8dab83

	if (range.attached && ! range.parentRange)
	range.detach();
	range.destroy();

The way it should work (and how it works when a view is destroyed by any other means) is the DOM is destroyed after the onDestroyed callback is called.

The solution, as far as I can tell, is to simply reverse the order of range.detach() and range.destroy().

 range.destroy();
 if (range.attached && ! range.parentRange)
   range.detach();

In my testing, this fixes the problem.

Note that this problem only applies to explicit calls to Blaze.remove(view). Destroying by other means all appears to work correctly.

Related

This problem, however, also seems to appear in the blaze-hot package.

blaze/packages/blaze-hot/update-templates.js

Lines 132 to 133 in a8dab83

	view._domrange.detach();
	view._domrange.destroy();

This problem occurs when updating the JS code of a template. When a hot module reload is performed on the template, the onDestroyed callback is called, but if it tries to reference the DOM, it will throw an error.

The solution is the same as the above, to switch the order of detach() and destroy(). When that is changed, the problem is fixed.

Let me know if this is not the right place to post the issue about blaze-hot.

Thank you!

[StorytellerCZ]

@jankapunkt @zodern thoughts?

[jankapunkt]

I can take a look at it tonight

[jankapunkt]

@michaelcbrook I tested this with 2.6.1-rc and if I change the code to your suggestions the views will not be removed anymore from the DOM and remain drawn on the screen.

Can you please give more context, why you need the view to remain attached to the DOM during onDestroyed callback?

[michaelcbrook]

@jankapunkt There are many times I use this, but one recent example I can give is when I initialized a jQuery plugin on a text input to let it autogrow when the text within it changes. The way the plugin works is it creates a hidden element at the end of the body mocking the current input, measures the width, and then updates the width of the original input to fit the content. To prevent memory leaks (from these hidden elements being added to the DOM), it requires that a destroy function is called when we don't care about it any longer. We call that in the onDestroyed callback.

Here's some actual code:

import inputAutogrow from 'input-autogrow';

Template.example.onRendered(function exampleOnRendered() {
  // initialize autogrow
  this.$('input').inputAutogrow({ maxWidth: 800, minWidth: 220 });
});

Template.example.onDestroyed(function exampleOnDestroyed() {
  // destroy autogrow
  this.$('input').inputAutogrow("destroy");
});

This caused the following error to be thrown and the hidden element was not removed from the DOM.

Error: Can't select in removed DomRange
    at Blaze._DOMRange.DOMRange.$ (http://localhost:3000/packages/blaze.js?hash=981d8eb98840b94fa27e85bfc8334660676125f3:654:26)
    at Blaze.TemplateInstance.$ (http://localhost:3000/packages/blaze.js?hash=981d8eb98840b94fa27e85bfc8334660676125f3:3450:25)
    at Blaze.TemplateInstance.exampleOnDestroyed (/imports/ui/layouts/example/components/example.js:197:10)
    at http://localhost:3000/packages/blaze.js?hash=981d8eb98840b94fa27e85bfc8334660676125f3:3279:20
    at Function.Template._withTemplateInstanceFunc (http://localhost:3000/packages/blaze.js?hash=981d8eb98840b94fa27e85bfc8334660676125f3:3662:14)
    at fireCallbacks (http://localhost:3000/packages/blaze.js?hash=981d8eb98840b94fa27e85bfc8334660676125f3:3275:12)
    at Blaze.View.<anonymous> (http://localhost:3000/packages/blaze.js?hash=981d8eb98840b94fa27e85bfc8334660676125f3:3382:5)
    at fireCallbacks (http://localhost:3000/packages/blaze.js?hash=981d8eb98840b94fa27e85bfc8334660676125f3:1874:75)
    at Object.Tracker.nonreactive (http://localhost:3000/packages/tracker.js?hash=5ef67b97eaf2ca907dc38459283f2349bada6814:625:12)
    at http://localhost:3000/packages/blaze.js?hash=981d8eb98840b94fa27e85bfc8334660676125f3:1871:13 undefined

With some monkey patching though, I was able to get it to work by overriding Blaze.remove and just switching the destroy() and detach() calls myself:

Blaze.remove = function (view) {
  if (! (view && (view._domrange instanceof Blaze._DOMRange)))
    throw new Error("Expected template rendered with Blaze.render");
  while (view) {
    if (! view.isDestroyed) {
      var range = view._domrange;
      range.destroy(); // THIS WAS ORIGINALLY AFTER THE IF STATEMENT BELOW (BUT NOT INSIDE IT)
      if (range.attached && ! range.parentRange)
        range.detach();
    }
    view = view._hasGeneratedParent && view.parentView;
  }
};

I think keeping the DOM attached during onDestroyed is warranted because that seems to be the intended functionality and the way that it works everywhere else.

Having said that, I tried several other solutions, like the one used inside flow-router-extra, but this created the same problem you mentioned of the DOM remaining drawn on the screen.

https://github.com/veliovgroup/flow-router/blob/1b7a7364bd36736ea3583e499483ebab1bc46a98/client/renderer.js#L13-L24

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

But this did not work for me, and I wonder if this was written in response to the Blaze.remove function not working correctly in the first place.

[jankapunkt]

Well, that's a problem now. I just changed the order in #366 because this caused lots of DOMRange not attached errors. Now I think this issue is connected to all this. Do you already use the 2.6.1-rc.1 release? If not, please try it out if it fixes your issue.
Otherwise I think we have to make another round of debugging to find the source of the issue.

[jankapunkt]

@michaelcbrook please see #374

Actually your propsed solution works, what was problematic was that I also use flow-router-extra which made my drawing faulty but with a clean project this works. In turn I will have to make a PR to the router to fix this.

[michaelcbrook]

I just tried running 2.6.1-rc.1 just to check, and without the fix I posted, the problem still exists. I also tested the fix on 2.6.1-rc.1 and it worked correctly. So I'm in support of PR #374 still being pushed through.

[jankapunkt]

I'm closing this as it's covered by release 2.6.1. Feel free to reopen when the issue persists in 2.6.1