Opened 6 years ago

Last modified 4 years ago

#18801 new defect

dijit/Destroyable::own dies horribly if owning things without a cleanup method

Reported by: primejunta Owned by:
Priority: undecided Milestone: 1.14
Component: Dijit Version: 1.11.0-rc1
Keywords: Cc:
Blocked By: Blocking:


To reproduce:

  1. Create a widget or other Destroyable, and .own something without a ::remove, ::destroy, or ::destroyRecursive method
  2. Call that widget's ::destroy method

Expected: widget is destroyed, the .own'ed object is ignored Observed: the destroy sequence fails with "TypeError?: handle[destroyMethodName] is not a function."

Note: it kind of makes sense that it works like this, but breaks back-compatibility. I'm running into this issue when setting listeners on a third-party AMD module which has an ::on method which doesn't return an object with a ::remove method. When I set a listener with dojo/on, it gets delegated to the third-party, and I can't ::own the on as usual. So it would be nice if this was fixed; all it would take is changing line 54 in dojo/Destroyable to:

if(typeof handle[destroyMethodName] == "function") handle[destroyMethodName](preserveDom);

Change History (7)

comment:1 Changed 6 years ago by bill

Component: GeneralDijit
Status: newpending

It's a pretty simple change, but does the current code really break back-compatibility? What code are you executing exactly that breaks, but used to work?

comment:2 Changed 6 years ago by primejunta

Status: pendingnew

As I mentioned in the description, it has to do with third-party AMD modules.

Specifically, I'm using Ace Editor through a wrapper widget I've created, and I'm setting watchers on the editor with dojo/on. The trouble is that dojo/on introspects the editor instance and notices that it has an ::on method of its own, so instead of using its own, it delegates setting the listener to that. The Editor's ::on method however doesn't return a function with a cleanup method.

So, what happens is that when I this.own( on( editor, "change", handler ) ) and then ::destroy the wrapper widget, it dies when trying to remove the listener.

This is arguably a problem with dojo/on -- if I could force it to use its own method to set the listener rather than delegating it to the editor, that would solve the problem too and would -- likely -- be a better solution.

I realise that .own'ing it won't actually do anything, but in Dojo 1.9.7 from which I'm upgrading the ::destroy sequence didn't break because of this.

As I said, not a big deal and I've already worked around it, but I thought it was reporting anyway.

comment:3 Changed 6 years ago by bill

I see, so in Dojo 1.9.7, this.own(randomObject) would just be a no-op and now it throws an error, albeit the throw is at the wrong time, i.e. when you call destroy() instead of when you call this.own(). I can see how that broke your app, although I don't know if it's technically a break in backwards compatibility since this.own() is still working according to spec. Depends on how you define backwards compatibility.

As per your other points, I agree there's sort of an incompatibility between dojo/on and the Ace Editor. And also, it seems like instead of doing:

this.own( on( editor, "change", handler ) )

that you should just be doing:

editor.on("change", handler);

or if you really need to deactivate that listener on destroy, then something like:

var thirdPartyHandle = editor.on("change", handler);
    destroy: function () { thirdPartyHandle.otherReleaseMethod(); }

comment:4 in reply to:  3 Changed 6 years ago by primejunta

Yep, that's correct. Also your way of handling the cleanup is cleverer than mine, I just extended my destroy method. Thanks!

comment:5 Changed 5 years ago by dylan

Milestone: tbd1.13

comment:6 Changed 4 years ago by dylan

Milestone: 1.131.14

comment:7 Changed 4 years ago by Anuj

So, what is the best way to use this.own() with 1.10.x, I have just moved from 1.9.3 and facing a similar issue

I am creating widgets programmatically

e.g. inside a widget I would do this

this.countDownWidget = new CountDown();

Is this not correct ? Please confirm. I've got a large app and changing the syntax everywhere would not be so simple.

Thanks, Anuj

Note: See TracTickets for help on using tickets.