thanhlong203 / closure-library

Automatically exported from code.google.com/p/closure-library
0 stars 0 forks source link

Expand API of goog.Disposable #328

Closed GoogleCodeExporter closed 8 years ago

GoogleCodeExporter commented 8 years ago
I think that goog.Disposable could use a few minor improvements, as 
demonstrated in my code review.

First, when I have several disposable fields in a class (some of which may 
never have been set), I often have something like:

/** @inheritDoc */
MyClass.prorotype.disposeInternal = function() {
  goog.base(this, 'disposeInternal');
  goog.dispose(this.foo);
  goog.dispose(this.bar);
  goog.dispose(this.baz);
};

With my proposed change, this could become:

/** @inheritDoc */
MyClass.prorotype.disposeInternal = function() {
  goog.base(this, 'disposeInternal');
  goog.disposeEvery(this.foo, this.bar, this.baz);
};

Or alternatively:

/** @inheritDoc */
MyClass.prorotype.disposeInternal = function() {
  goog.base(this, 'disposeInternal');
  goog.disposeAll([this.foo, this.bar, this.baz]);
};

Though in this case, the former is preferred, because the latter entails 
creating an array. I'm suggesting disposeAll() for the function that takes an 
array, much like addAll() is the method that takes an Iterable in a Java 
collection. I chose disposeEvery() instead of disposeEach() because the latter 
sounds similar to forEach() which operates on an array, whereas disposeEvery() 
is a vararg function.

The other change is to make it possible to link disposables, in that when one 
is disposed, the other is disposed. This addresses an issue that is described 
on p.135 of Closure: The Definitive Guide in which it can be ambiguous whether 
a disposable argument passed into a constructor should be disposed of by the 
class, or by the code that instantiated the class. In the case of the former, 
the caller can do:

var instance = new SomeClass(disposable);
instance.registerDisposable(disposable);

with the expectation that SomeClass will never dispose of "disposable" 
explicitly -- only as a side-effect of being registered via 
registerDisposable().

Code review: http://codereview.appspot.com/4532068/

Includes unit test.

Original issue reported on code.google.com by bolinf...@gmail.com on 17 May 2011 at 3:30

GoogleCodeExporter commented 8 years ago
IMO registerDisposable adds more bloat to the library than value.

Until now one had to remember to call dispose() on the child objects. Now they 
have to remember to call registerDisposable.

Original comment by pall...@google.com on 24 May 2011 at 2:59

GoogleCodeExporter commented 8 years ago
On the other hand objects usually don't only dispose of their children, but 
they also null them out. Even if they use registerDisposable, they have to 
implement disposeInternal as well.

Original comment by pall...@google.com on 24 May 2011 at 3:03

GoogleCodeExporter commented 8 years ago
Can you please stop marking my issues as WontFix before there is any sort of
open discussion on them? It would be much more helpful to add comments to
the code review first than to make a unilateral decision to close the issue
before other team members have a chance to look at it.

As you can see here, there are other Googlers who are interested in this
change:
http://groups.google.com/group/closure-library-discuss/browse_thread/thread/b7b5
6cf982f5473e

<http://groups.google.com/group/closure-library-discuss/browse_thread/thread/b7b
56cf982f5473e>You
only need to "null out" references to "children" when they are COM objects,
such as DOM nodes and XHRs on Internet Explorer. Because a subclass of
goog.Disposable is not a COM object, it will not be an issue for such
objects, which is the overwhelming majority of cases.

Original comment by bolinf...@gmail.com on 24 May 2011 at 7:46

GoogleCodeExporter commented 8 years ago

Original comment by Nicholas.J.Santos on 25 May 2011 at 3:13

GoogleCodeExporter commented 8 years ago
This issue was closed by revision r976.

Original comment by Nicholas.J.Santos on 27 May 2011 at 8:08