#493 Improve CSR deinitialization

[3rdvision]

-	-
Issue	https://github.com/ripe-tech/ripe-sdk/issues/493
Decisions	- Improve de-initialization logic by adding missing _destroyConfig, assigning proper event callback to allow proper unbinding match and avoid event handler duplication by making sure previous handlers are unbinded first before binding.

[3rdvision]

:warning: @3rdvision self reminder to release and integrate this in ripe-commons-pluginus and ripe-white after merge

[NFSS10]

@3rdvision The behaviour is intended, you don't need to do that as the owner is the one who handles those callbacks.

Those are handled in this: https://github.com/ripe-tech/ripe-sdk/blob/ed0607fc0613f3de98f3b73ebb2a9d85537e49df/src/js/visual/configurator-csr.js#L164

[3rdvision]

That behaviour is intended

What behavior is intended?

There are several things that need to be fixed:

• adding missing _destroyConfig that calls _unregisterConfigHandlers - currently _unregisterConfigHandlers is not being used
• assigning proper event callback to allow proper unbinding match (the current implementation doesn't unbind anything)
• avoid event handler duplication by making sure previous handlers are unbinded first before binding - this is more specific for chaotic post_config which is ripe-white's case but an extra guard adds more confidence that we always have only 1 callback for each bind

[NFSS10]

adding missing _destroyConfig that calls _unregisterConfigHandlers - currently _unregisterConfigHandlers is not being used

It's not missing anything, the _unregisterConfigHandlers() isn't called because there is no behaviour that handles the deinit() midway the configurator usage. This line handles that deinit ((line 164)): https://github.com/ripe-tech/ripe-sdk/blob/ed0607fc0613f3de98f3b73ebb2a9d85537e49df/src/js/visual/configurator-csr.js#L164

assigning proper event callback to allow proper unbinding match (the current implementation doesn't unbind anything)

It does, the binds are in the owner. That logic is in the observable and the configurator inherits that logic (Obersvable -> Interactable -> Visual -> Configurator). As I said above, deinit handles that.

avoid event handler duplication by making sure previous handlers are unbinded first before binding - this is more specific for chaotic post_config which is ripe-white's case but an extra guard adds more confidence that we always have only 1 callback for each bind

Don't know what this means but it shouldn't as the config handlers are only registered when you init a configurator thus that only happens once per configurator and for the initials bindings it's already handling as it reconstructs and reregisters itself in mid usage.

adding missing _destroyConfig that calls _unregisterConfigHandlers - currently _unregisterConfigHandlers is not being used

Same as the first point.

[3rdvision]

I'll separate by points otherwise this will be hard to track.

---

1-

It's not missing anything, the _unregisterConfigHandlers() isn't called because there is no behaviour that handles the deinit() midway the configurator usage. This line handles that deinit ((line 164)):

_unregisterConfigHandlers is dead code. It's not being called anywhere. ripe.Visual.prototype.deinit.call(this); does not call it. I'm not sure what you're suggesting but deinit should unregister all the event bindings and it's not doing it. Feel free to suggest another fix.

---

2-

It does, the binds are in the owner. That logic is in the observable and the configurator inherits that logic (Obersvable -> Interactable -> Visual -> Configurator). As I said above, deinit handles that.

When doing unbind with this.owner && this.owner.unbind("initials_extra", this._onInitialsExtraEvent);, we call this removeCallback which then does this check. The check never finds that method because it wasn't that method that was binded, it was a new arrow function of that method but that has a difference reference.

The way it's currently implemented in configurator-csr, it doesn't find any reference of this._onInitialsExtraEvent because the actual function that was saved in the callback array was a new reference for an arrow function done with

  this.owner.bind("initials_extra", (initialsExtra, params) =>
      this._onInitialsExtraEvent(this, initialsExtra, params)
  );

The reference that is saved when receiving (initialsExtra, params) => this._onInitialsExtraEvent(this, initialsExtra, params) in the args is different than receiving this._onInitialsExtraEvent.

Also, when I reviewed this at first, I didn't caught but we should follow the same methodology done in other places like here. The bind is done the correct way - it binds the callback, saves it globally inside this._partBind and later does this.owner.unbind("part", this._partBind);.

to better understand this you can try in your browser to do

func1 = () => {}
func1 === (() => func1) // this returns false because the reference of a new arrow func is not the same as the function itself

---

Don't know what this means but it shouldn't as the config handlers are only registered when you init a configurator thus that only happens once per configurator and for the initials bindings it's already handling as it reconstructs and reregisters itself in mid usage.

It should only happen once per configurator, unfortunately that is not the case. This was a very tricky one to debug and understand. The weird behavior that happens is because post_config is called twice in some scenarios and because of it 2 handlers will be binded and only one will be unbinded after deinit. How do you suggest do fix this then?

Part of the issue is because CSR only initializes after a post_config event. And in some scenarios you may already have a model configurated before binding the configurator. This is the case for ripe-white.

[NFSS10]

_unregisterConfigHandlers() being dead code is a yes in the sense of it not being used, but a no because it's not being used because at the current state there is no behaviour that needes to call it mid lifecycle of the CSR configurator so it's just an unused method waiting to be called when needed.

What I'm saying is that the Visual.deinit will reach this: https://github.com/ripe-tech/ripe-sdk/blob/7a07234131d81c2445b1aae9b2f8a66a755d80ae/src/js/base/observable.js#L33 and clear all callbacks as it's supposed to.

About what you are saying, to see if I'm understanding correctly. You are essently saying that the Visual.deinit is not correctly unbinding the owner binds? We need to know exactly the problem because we had some issues a few years ago related to this area (one of those related to thumbnails not being properly discarded) and the issue was because essentially the SDK logic is not well built and some of the components where using it's logic incorrectly.

It's important to know the situation because It's not the Configurator responsibility to explicitly clear the owner binds on deinit() that's why we call the parent method. Doing that would be redundant or a symptom of a bug somewhere so we have to make sure. The only cases where we want to explicitly do owner unbinds is if we have some kind of behaviour that will need to do it mid cycle of the configurator lifecycle and it seems is not the case here.

[NFSS10]

Ok, scratch the last part, it's being used in _deinitCsr and that's used mid lifecycle, my bad!

[3rdvision]

_deinitCsr() is only called when deInit() is called on the configurator lifecycle, thus the points still stand.

This is true but these bindings are not properly being unbinded and later as we can see in the video demos, they will throw "null pointer" problems.

[vercel[bot]]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated
ripe-sdk	✅ Ready (Inspect)	Visit Preview	💬 Add your feedback	Mar 8, 2023 at 4:05PM (UTC)

[3rdvision]

@NFSS10 I pushed the changes we discussed in person.

Let me know if there's anything missing.