Binding twice after reconnect

[KoenLav]

Hi Kristian,

We are happily using your package for a while now. But since a while it seems that events are binded to HTML nodes twice after Meteor reconnects.

Have you seen this problem before?

Where is the code located which binds the event (within your package)? Maybe I can take a look ;)

[wvanooijen92]

I am running into the same problem...

[KoenLav]

More specifically: the click binding will get executed twice (or thrice, etc.) when the connection is lost.

I think the package expects the unbind function to be called when the elements are removed as a result of the data being unavailable, but we use GroundDB to ensure the data is available even when offline.

It seems the bind function is called again on reconnect, but the previous bind is not removed. (unbind is not called)

The strange this is that this seems to happen on some templates, but not others...

[KoenLav]

Note: I added a custom mousedown binding and the same occurs.

Note: when I console.log(this) within the viewmodel I can see the functions get executed after reconnection and a second nexus is added, but the first one is not removed.

Some pointers would be greatly appreciated :)

[dalgard]

Thanks for the debugging effort. I'll look into it first thing, probably later today.

[dalgard]

@wvanooijen92 Do you use GroundDB as well?

[KoenLav]

Basically the onReady gets executed, but the onInvalidate does not.

Is it possible that in

const computation = Tracker.currentComputation;

if (computation)
  computation.onInvalidate(callback);

}

Tracker.currentComputation is undefined?

[KoenLav]

I'm pretty sure it is (null, not undefined), but now on to the why...

Could it be that because the datasource is GroundDB the computation is different?

But then why doesn't this happen on some of our other pages (where the datasource is also GroundDB, I verified it doesn't happen)...

[KoenLav]

I can verify that the unbind function is actually called on these elements after Meteor.reconnect() and ends up all the way at elem.removeEventListener with elem, type and listener properly defined (as far as I can tell).

So maybe it's not the unbinding going wrong, but the binding executing twice? Looking into that now.

[KoenLav]

Nope, bind gets called only once after reconnect, so I'm back to looking at the removeEventListener and why it's not working...

[KoenLav]

  console.log(this.view)

  this.view[ViewModel.nexusesKey].remove(this);

  console.log(this.view)

Both view objects are identical and have identical contents (so no nexus is removed).

Is it possible that the listener has changed, which is why removeEventListener is not working and which would also explain by .remove(this) is not working (if it removes based on matching the object in an array)?

[KoenLav]

I also noticed that the vm-bind-id of all elements changes every time after calling meteor.reconnect() (this also happens on elements where we do not experience the multiple bindings problem).

[dalgard]

If you set a custom attribute on one of those elements in dev tools, does the attribute disappear after the reconnect, meaning that the element is re-rendered?

[dalgard]

I swapped jQuery for vanilla removeEventListener in 1.0.0 – would it be possible for you to downgrade to 0.9.4 and check if the problem is there, too? I suspect that it's not.

[KoenLav]

I tried downgrading to 0.9.4 indeed, the problem persisted.

[dalgard]

Hum...

[dalgard]

Let me know about the custom attributes. I definitely think you've found the correct part of the code to look at, but I'm having trouble reproducing the bug simply with Meteor.disconnect(), Meteor.reconnect(). The ids don't change in my page.

Something to do with computations is a good bet...

[KoenLav]

Yeah, the thing is it only happens on one set of elements in our code, all other sets are unaffected, but I don't see anything in those elements which is 'strange'. Would it help you to have a description of the element object?

Checking the custom attribute now (also re-added jQuery as a dependency and manually change the code back to jQuery in your package to test).

The IDs do change on all elements in our app, so it seems this might be caused by one 'bug' in combination with another...

[dalgard]

Good point, but in that case @wvanooijen92 has the same combination.

[KoenLav]

wvanooijen92 is working on the same project... Don't know why he hasn't responded, he was also troubleshooting this.

I checked: when adding a custom attribute the attribute remains (indicating the element is not rerendered), but the vm-bind-id does change.

[KoenLav]

By the way: the vm-bind-id of ALL elements changes (not just those who depend on a GroundDB collection, also those who don't depend on a collection at all).

And damn, I forgot, but we are using GroundDB at ground:[email protected]

[KoenLav]

Apologies for the inconenience :(

[KoenLav]

I just also tried switching back to jQuery (re-adding it as a dependency and using the .on and .off methods rather than the eventListener methods in plain Javascript but the problem persists.

I think the the problem of multiple bindings is VERY specific and only happens when elements are somehow 're-evaluated' by your package, causing the vm-bind-id to change. I am however yet to identify the difference between most elements in our project and these specific elements.

[KoenLav]

Also I'm using Meteor 1.2.1

[dalgard]

No reason for apology. It's definitely caused by some detail that has to do with the computation – what you call re-evaluation.

[dalgard]

The hooks below should unbind the element, before it is bound anew.

    this.onRefreshed(this.unbind);
    this.onDestroyed(this.unbind);
    this.onInvalidate(() => this.unbind(true));

Maybe one of these hooks doesn't run. Would you mind checking onRefreshed with a breakpoint on base.js#L102?

If the unbind function is called – please check which hook calls it by looking one level up in the call stack with a breakpoint on nexus.js#L237 – maybe the do_unbind parameter doesn't evaluate to true for some reason?

[KoenLav]

I can do you one better: the onInvalidate hook actually runs, the do_unbind
value is true and it even ends up all the way at removeEventListener (with
the (seemingly) correct element, binding type and listener, but it does not
get removed.
Op 15 mrt. 2016 10:23 a.m. schreef "Kristian Dalgård" <
[email protected]>:

The hooks below (declared in the abstract Base class) should unbind the
element, before it is bound anew. Maybe one of these hooks doesn't run.
Would you mind checking that with a breakpoint on base.js#L102
https://github.com/dalgard/meteor-viewmodel/blob/master/packages/dalgard_viewmodel/lib/base.js#L102
?

this.onRefreshed(this.unbind);
this.onDestroyed(this.unbind);
this.onInvalidate(() => this.unbind(true));

If unbind is called (breakpoint on nexus.js#L237
https://github.com/dalgard/meteor-viewmodel/blob/master/packages/dalgard_viewmodel/lib/nexus.js#L237),
maybe its do_unbind parameter doesn't evaluate to true for some reason.

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub:

#20 (comment)

[dalgard]

Oh, right – you already said that...

[dalgard]

Maybe a new bind is triggered prematurely so that this.listener gets overwritten with a new listener, before unbind get a chance to unregister the old this.listener?

You might be able to debug that if you set a breakpoint where the listener is created (inside bind) and then right click the listener and select Store as global variable. Then you can compare that variable (temp1) with the listener that is unregistered in unbind.

(I have a tendency to edit my posts right after sending them, would it be possible for you to read directly on GitHub?)

[dalgard]

I guess it's simple – a second call to bind should never occur unless unbind has run successfully.

[KoenLav]

Just the one I read on my phone (currently doing research for my thesis, so have to multitask a little).

I guess that makes sense (not calling a second bind on an element with the same type and same listener), do you still need me to check which method calls the unbind?

[KoenLav]

Working on that breakpoint now

[dalgard]

For sure, it could happen in other scenarios – thanks again for taking the time to debug this. I'll look into the reason why do_unbind evaluates to false.

The {{bind}} helper being triggered twice is expected behavior – dynamic attributes are re-run when anything about the element changes, which is the reason for many of the tricky maneuvers employed by the library.

Just to be sure, you are using the latest version 1.0.2, right?

[KoenLav]

I'm not really used to working with the stack directly, but the when I set a breakpoint on that line and look in the Chrome DevTools the second line says "nexus.js:111" which is this.onReady(this.bind) in the Nexus constructor.

[KoenLav]

The false I mention there is not the do_unbind, it is the result of this.view[ViewModel.nexusesKey].remove(this);

[dalgard]

The nexuses list on the view is only for debugging and doesn't affect the bindings.

[KoenLav]

Do you think I should focus on why this.view[ViewModel.nexusesKey].remove(this) returns false or rather on why the bindHelper is called twice.

There are two differences between elements that don't have the erronous behavior and those who do. For elements that have the bug:

• the bindHelper is called twice after reconnect;
• the first unbind results in a false when calling this.view[ViewModel.nexusesKey].remove(this).

[KoenLav]

I see, if that is the case then Nexus.remove(this) a few lines earlier would probably also return false. I'll look into that.

[dalgard]

It's weird if it returns false, but those two lines have no bearing on the bindings themselves, they only track what is currently on the page for debugging purposes.

[dalgard]

The bind helper being called twice is expected behavior and only causes problems when the previous binding isn't unbound properly before each subsequent call.

[KoenLav]

Nexus.remove(this) also returns false.

But I see your point.

I do still think the afterFlush ==> bind being called twice is the culprit. Every time reconnect is called the previous binding is removed, but two new ones are added due to the afterFlush being triggered twice.

afterFlush should never trigger a bind if the previous bind has not yet been removed, I guess.

[KoenLav]

Actually I'm 100% sure this is the issue.

Even though Nexus.remove(this) returns false, before that the eventListener is removed so that should be fine (in terms of functionality, not in terms of memory leaks).

However because afterFlush is set twice (because it is bound in two different computations, one for the first triggering of bindHelper and another for the second) it is executed twice, but this shouldn't happen.

I'm still not sure whether afterFlush is the right 'tool for the job' (but I'm not sufficiently equipped to think of a better alternative).

[dalgard]

Here's another idea: unbind accidentally runs before bind does, which would explain why Nexus.remove(this) returns false – it can't be removed from the global list, because it hasn't been added to it yet (which happens in bind).

The first unbind ought to run after the first bind, of course.

Don't pay too much attention to the multiple calls to bind/unbind, by the way – it's the only way that the library can be guaranteed to work in all situations and what makes it especially robust, in all modesty. Except in your case, of course ;)

[dalgard]

I wonder whether view.onViewDestroyed sometimes runs before view.onViewReady? If the view is destroyed immediately, is the unbind callback run right away when registered? In that case, the value of view.isRendered and view.isDestroyed is important...

[dalgard]

Still wish I had a test case ;)

[dalgard]

To muddle the image, list.add and list.remove will both trigger a Tracker flush.

[dalgard]

Have we established exactly which hook is responsible for running unbind? Is it onRefreshed, onDestroyed, or onInvalidate?

[KoenLav]

onDestroyed

[KoenLav]

I am using these console.logs:

this.onRefreshed(function() {
  if (Meteor.checkElement(view)) {
    console.log("onRefreshed")
  }

  that.unbind()
});

// Unbind element on view destroyed
this.onDestroyed(function() {
  if (Meteor.checkElement(view)) {
    console.log("onDestroyed")
  }

  that.unbind()
});

// Unbind element on computation invalidation
this.onInvalidate(function() {
  if (Meteor.checkElement(view)) {
    console.log("onDestroyed")
  }

  that.unbind(true)
});

In the Nexus constructor.

[dalgard]

👍

[KoenLav]

I wish I could create a test case, but I have yet to be able to reproduce this in any other setting...

I'm going to try to 'cut out' the code causing this, but to be honest I expect it would just start working when I isolate it (going to try to do so anyway).

[dalgard]

Try wrapping the bind passed to onReady (inside the Nexus constructor) like you've done above, and have it output to console.log, too – and then add the nexus selector to that log as well as the rest of them – what do you get?

// Bind element on view ready
this.onReady(() => {
  // What does Meteor.checkElement mean, btw?
  console.log("onReady", this.selector);

  this.bind();
});

[dalgard]

In other words, does unbind seem to run before bind for the same selector?

[KoenLav]

I use the checkElement function to filter the console.logs to just one element.

[KoenLav]

First rendering of the element:

click: addProduct
viewmodel.js:510 =====
viewmodel.js:512 newNexus
nexus.js:120 =====
2base.js:81 onViewReady
nexus.js:113 onReady [vm-bind-id='128']
nexus.js:261 bind

Destroing of the element (caused by reconnect):
onDestroyed [vm-bind-id='128']
nexus.js:275 unbind
nexus.js:311 true

Second rendering of the element (caused by reconnect):

click: addProduct
viewmodel.js:510 =====
viewmodel.js:512 newNexus
nexus.js:120 =====
nexus.js:103 onDestroyed [vm-bind-id='259']
nexus.js:275 unbind
nexus.js:311 false
base.js:66 afterFlush
nexus.js:113 onReady [vm-bind-id='259']
nexus.js:261 bind
viewmodel.js:483 click: addProduct
viewmodel.js:510 =====
viewmodel.js:512 newNexus
nexus.js:120 =====
base.js:66 afterFlush
nexus.js:113 onReady [vm-bind-id='287']
nexus.js:261 bind

So you are right, the unbind on 259 is called before the bind is called.

[dalgard]

Just as I suspected. Interesting...

[KoenLav]

Terribly sorry, I made a mistake in the copying of the console.logs, it's not the onDestroyed callback causing this, it's the onInvalidate (which makes much, much more sense).

[dalgard]

Right. Still...

[KoenLav]

Ok, I think I've written a fix.

I will be cleaning up my code and verifying whether it works in multiple scenario's.

[KoenLav]

I submitted a pull request, I had to hurry a little so it is not thoroughly tested, but the basic idea should be clear:

When a view becomes ready, we set viewModelReady to true.

If we invalidate the view, it is no longer ready. When we then try to invalidate it again, we move the unbinding to the onReady.