From 48211566ad524b5f9551cc40707837d7bb761b3e Mon Sep 17 00:00:00 2001 From: Dmitry S Date: Sun, 4 Feb 2018 18:38:42 -0500 Subject: [PATCH 1/3] Redesign dispose library in a big way --- lib/_domComponent.ts | 55 ++++--- lib/dispose.ts | 357 ++++++++++++++++++++++++------------------- lib/domevent.ts | 6 +- lib/emit.ts | 8 +- package.json | 3 +- test/dispose.js | 62 ++++---- test/dom.js | 13 +- 7 files changed, 285 insertions(+), 219 deletions(-) diff --git a/lib/_domComponent.ts b/lib/_domComponent.ts index 53aeccf..e1c8543 100644 --- a/lib/_domComponent.ts +++ b/lib/_domComponent.ts @@ -5,7 +5,7 @@ import {domDispose, onDisposeElem} from './_domDispose'; import {DomElementArg, DomElementMethod, update} from './_domImpl'; -import {Disposable} from './dispose'; +import {Disposable, IDisposableOwner} from './dispose'; // Use the browser globals in a way that allows replacing them with mocks in tests. import {G} from './browserGlobals'; @@ -18,10 +18,21 @@ import {G} from './browserGlobals'; * In addition, a "class" component may be disposed to remove it from the DOM, although this is * uncommon since a UI component is normally owned by its containing DOM. */ -export class Component extends Disposable { +export abstract class Component extends Disposable { private _markerPre: Node|undefined; private _markerPost: Node|undefined; + /** + * Component should implement render(). If overriding constructor, remember to pass along + * arguments, and keep in mind that render() will be called before additional constructor code. + * TODO This should have typescript overloads to ensure that it takes the same arguments as + * render(). + */ + constructor(elem: Element, ...args: any[]) { + super(); + this._mount(elem, this.render(...args)); + } + /** * Components must extend this class and implement a `render()` method, which is called at * construction with constructor arguments, and should return DOM for the component. @@ -32,16 +43,13 @@ export class Component extends Disposable { * DOM element, a string, null, or an array. The returned DOM is automatically owned by the * component, so do not wrap it in `this.autoDispose()`. */ - public render(...args: any[]): DomElementArg { - throw new Error("Not implemented"); - } + public abstract render(...args: any[]): DomElementArg; /** - * This is not intended to be called directly or overridden. Instead, implement render(). + * Inserts the content into DOM, arranging for it to be disposed when this Component is, and to + * dispose the Component when the parent element gets disposed. */ - protected create(elem: Element, ...args: any[]) { - const content: DomElementArg = this.render(...args); - + private _mount(elem: Element, content: DomElementArg): void { this._markerPre = G.document.createComment('A'); this._markerPost = G.document.createComment('B'); @@ -55,7 +63,7 @@ export class Component extends Disposable { // When the component is disposed, unmount the DOM we created (i.e. dispose and remove). // Except that we skip this as unnecessary when the disposal is triggered by containing DOM. - this.autoDisposeWith(this._unmount, this); + this.onDispose(this._unmount, this); // Insert the result of render() into the given parent element. update(elem, this._markerPre, content, this._markerPost); @@ -64,8 +72,9 @@ export class Component extends Disposable { /** * Detaches and disposes the DOM created and attached in _mount(). */ - private _unmount() { - // Dispose the owned content, and remove it from the DOM. + private _unmount(): void { + // Dispose the owned content, and remove it from the DOM. The conditional skips the work when + // the unmounting is triggered by the disposal of the containing DOM. if (this._markerPre && this._markerPre.parentNode) { let next; const elem = this._markerPre.parentNode; @@ -80,11 +89,14 @@ export class Component extends Disposable { } } -export type ComponentClassType = typeof Component; +export interface IComponentClassType { + new (...args: any[]): T; + create(owner: IDisposableOwner|null, ...args: any[]): T; +} /** * Construct and insert a UI component into the given DOM element. The component must extend - * dom.Component(...), and must implement a `render(...)` method which should do any constructor + * dom.Component, and must implement a `render(...)` method which should do any rendering work * work and return DOM. DOM may be any type value accepted by dom() as an argument, including a * DOM element, string, null, or array. The returned DOM is automatically owned by the component. * @@ -110,13 +122,12 @@ export type ComponentClassType = typeof Component; * dom.Component(...) and implement the render() method. * @param {Objects} ...args: Arguments to the constructor which passes them to the render method. */ -export function createElem(elem: Element, ComponentClass: ComponentClassType, ...args: any[]) { - // tslint:disable-next-line:no-unused-expression - new ComponentClass(elem, ...args); +export function createElem(elem: Element, cls: IComponentClassType, ...args: any[]): T { + return cls.create(null, elem, ...args); } -export function create(ComponentClass: ComponentClassType, ...args: any[]): DomElementMethod { +export function create(cls: IComponentClassType, ...args: any[]): DomElementMethod { // tslint:disable-next-line:no-unused-expression - return (elem) => { new ComponentClass(elem, ...args); }; + return (elem) => { cls.create(null, elem, ...args); }; } /** @@ -131,10 +142,10 @@ export function create(ComponentClass: ComponentClassType, ...args: any[]): DomE * soon as it's created, so an exception in the init function or later among dom()'s arguments * will trigger a cleanup. */ -export function createInit(ComponentClass: ComponentClassType, ...args: any[]): DomElementMethod { +export function createInit(cls: IComponentClassType, ...args: any[]): DomElementMethod { return (elem) => { - const initFunc: (c: Component) => void = args.pop(); - const c = new ComponentClass(elem, ...args); + const initFunc: (c: T) => void = args.pop(); + const c = cls.create(null, elem, ...args); initFunc(c); }; } diff --git a/lib/dispose.ts b/lib/dispose.ts index 6d92fff..8f5bcf2 100644 --- a/lib/dispose.ts +++ b/lib/dispose.ts @@ -4,128 +4,151 @@ * * https://phab.getgrist.com/w/disposal/ * - * Disposable is a class for components that need cleanup (e.g. maintain DOM, listen to - * events, subscribe to anything). It provides a .dispose() method that should be called to - * destroy the component, and .autoDispose() family of methods that the component should use to - * take responsibility for other pieces that require cleanup. + * Disposable is a class for components that need cleanup (e.g. maintain DOM, listen to events, + * subscribe to anything). It provides a .dispose() method that should be called to destroy the + * component, and .onDispose()/.autoDispose() methods that the component should use to take + * responsibility for other pieces that require cleanup. * * To define a disposable class: - * class Foo extends Disposable { - * create(...args) { ...constructor work... } // Instead of constructor, if needed. - * } + * class Foo extends Disposable { ... } * * To create Foo: - * let foo = new Foo(args...); + * const foo = Foo.create(owner, ...args); + * This is better than `new Foo` for two reasons: + * 1. If Foo's constructor throws an exception, any disposals registered in that constructor + * before the exception are honored. + * 2. It ensures you specify the owner of the new instance (but you can use null to skip it). * - * Foo should do constructor work in its create() method (or rarely other methods), where it can - * take ownership of other objects: - * this.bar = this.autoDispose(new Bar(...)); + * In Foo's constructor (or rarely methods), take ownership of other Disposable objects: + * this.bar = Bar.create(this, ...); * - * Note that create() is automatically called at construction. Its advantage is that if it throws - * an exception, any calls to .autoDispose() that happened before the exception are honored. + * For objects that are not instances of Disposable but have a .dispose() methods, use: + * this.bar = this.autoDispose(createSomethingDisposable()); * - * For more customized disposal: - * this.baz = this.autoDisposeWithMethod('destroy', new Baz()); - * this.elem = this.autoDisposeWith(ko.cleanNode, document.createElement(...)); - * When `this` is disposed, it will call this.baz.destroy(), and ko.cleanNode(this.elem). + * To call a function on disposal (e.g. to add custom disposal logic): + * this.onDispose(() => this.myUnsubscribeAllMethod()); + * this.onDispose(this.myUnsubscribeAllMethod, this); // slightly more efficient * - * To call another method on disposal (e.g. to add custom disposal logic): - * this.autoDisposeCallback(this.myUnsubscribeAllMethod); - * The method will be called with `this` as context, and no arguments. - * - * To wipe out this object on disposal (i.e. set all properties to null): + * To mark this object to be wiped out on disposal (i.e. set all properties to null): * this.wipeOnDispose(); * See the documentation of that method for more info. * - * To dispose Foo: + * To dispose Foo directly: * foo.dispose(); - * Owned objects will be disposed in reverse order from which `autoDispose` were called. - * - * To release an owned object: - * this.disposeRelease(this.bar); - * - * To dispose an owned object early: - * this.disposeDiscard(this.bar); - * * To determine if an object has already been disposed: * foo.isDisposed() + * + * If you need to replace an owned object, or release, or dispose it early, use a Holder: + * this._holder = Holder.create(this); + * Bar.create(this._holder, 1); // creates new Bar(1) + * Bar.create(this._holder, 2); // creates new Bar(2) and disposes previous object + * this._holder.clear(); // disposes contained object + * this._holder.release(); // releases contained object + * + * If creating your own class with a dispose() method, do NOT throw exceptions from dispose(). + * These cannot be handled properly in all cases. Read here about the same issue in C++: + * http://bin-login.name/ftp/pub/docs/programming_languages/cpp/cffective_cpp/MAGAZINE/SU_FRAME.HTM#destruct */ +import {LLink} from './emit'; + +/** + * Anything with a .dispose() method is a disposable object, and implements the IDisposable interface. + */ export interface IDisposable { dispose(): void; } -interface IDisposalEntry { - disposer: (obj: any) => void; - obj: any; +/** + * Anything with .autoDispose() can be the owner of a disposable object. + */ +export interface IDisposableOwner { + autoDispose(obj: IDisposable): void; } -export abstract class Disposable implements IDisposable { - private _disposalList: IDisposalEntry[]; +// Internal "owner" of disposable objects which doesn't actually dispose or keep track of them. It +// is the effective owner when creating a Disposable with `new Foo()` rather than `Foo.create()`. +const _noopOwner: IDisposableOwner = { + autoDispose(obj: IDisposable): void { /* noop */ }, +}; +// Newly-created Disposable instances will have this as their owner. This is not a constant, it +// is used by create() for the safe creation of Disposables. +let _defaultDisposableOwner = _noopOwner; + +/** + * Base class for disposable objects that can own other objects. See the module documentation. + */ +export abstract class Disposable implements IDisposable, IDisposableOwner { /** - * Constructor forwards arguments to `this.create(...args)`, which is where subclasses should - * do any constructor work. This ensures that if create() throws an exception, dispose() gets - * called to clean up the partially-constructed object. + * Create Disposable instances using `Class.create(owner, ...)` rather than `new Class(...)`. + * + * This reminds you to provide an owner, and ensures that if the constructor throws an + * exception, dispose() gets called to clean up the partially-constructed object. + * + * Owner may be null if intend to ensure disposal some other way. + * + * TODO: create() needs more unittests, including to ensure that TypeScript types are done + * correctly. */ - constructor(...args: any[]) { - this._disposalList = []; - + // The complex-looking overloads are to ensure that it can do type-checking for constuctors of + // different arity. E.g. if Foo's constructor takes (number, string), we want Foo.create to + // require (owner, number, string) as arguments. + public static create(this: new () => T, owner: IDisposableOwner|null): T; + public static create(this: new (a: A) => T, owner: IDisposableOwner|null, a: A): T; + public static create(this: new (a: A, b: B) => T, owner: IDisposableOwner|null, a: A, b: B): T; + public static create(this: new (a: A, b: B, c: C) => T, owner: IDisposableOwner|null, + a: A, b: B, c: C): T; + public static create(this: new (a: A, b: B, c: C, d: D) => T, owner: IDisposableOwner|null, + a: A, b: B, c: C, d: D): T; + public static create(this: new (a: A, b: B, c: C, d: D, e: E) => T, owner: IDisposableOwner|null, + a: A, b: B, c: C, d: D, e: E): T; + public static create(this: new (...args: any[]) => T, owner: IDisposableOwner|null, + ...args: any[]): T { + const origDefaultOwner = _defaultDisposableOwner; + const holder = new Holder(); try { - this.create(...args); + // The newly-created object will have holder as its owner. + _defaultDisposableOwner = holder; + return _autoDispose(owner, new this(...args)); } catch (e) { try { - this.dispose(); - } catch (e) { + // This calls dispose on the partially-constructed object + holder.clear(); + } catch (e2) { // tslint:disable-next-line:no-console - console.error("Error disposing partially constructed %s:", this.constructor.name, e); + console.error("Error disposing partially constructed %s:", this.name, e2); } throw e; + } finally { + // On success, the new object has a new owner, and we release it from holder. + // On error, the holder has been cleared, and the release() is a no-op. + holder.release(); + _defaultDisposableOwner = origDefaultOwner; } } - /** - * Take ownership of `obj`, and dispose it when `this.dispose` is called. - * @param {Object} obj: Disposable object to take ownership of. - * @returns {Object} obj - */ - public autoDispose(obj: T): T { - return this.autoDisposeWith(_defaultDisposer, obj); - } + private _disposalList: DisposalList = new DisposalList(); - /** - * Take ownership of `obj`, and dispose it by calling the specified function. - * @param {Function} disposer: disposer(obj) will be called to dispose the object, with `this` - * as the context. - * @param {Object} obj: Object to take ownership of, on which `disposer` will be called. - * @returns {Object} obj - */ - public autoDisposeWith(disposer: (obj: T) => void, obj: T): T { - this._disposalList.push({obj, disposer}); - return obj; + constructor() { + // This registers with a temp Holder when using create(), and is a no-op when using `new Foo`. + _defaultDisposableOwner.autoDispose(this); } - /** - * Take ownership of `obj`, and dispose it with `obj[methodName]()`. - * @param {String} methodName: method name to call on obj when it's time to dispose it. - * @returns {Object} obj - */ - public autoDisposeWithMethod(methodName: string, obj: T): T { - return this.autoDisposeWith((_obj: any) => _obj[methodName](), obj); + /** Take ownership of obj, and dispose it when this.dispose() is called. */ + public autoDispose(obj: T): T { + this.onDispose(obj.dispose, obj); + return obj; } - /** - * Adds the given callback to be called when `this.dispose` is called. - * @param {Function} callback: Called on disposal with `this` as the context and no arguments. - * @returns nothing - */ - public autoDisposeCallback(callback: () => void): void { - this.autoDisposeWith(_callFuncHelper, callback); + /** Call the given callback when this.dispose() is called. */ + public onDispose(callback: (this: T) => void, context?: T): void { + this._disposalList.addListener(callback, context); } /** * Wipe out this object when it is disposed, i.e. set all its properties to null. It is - * recommended to call this early in the constructor. It's safe to call multiple times. + * recommended to call this early in the constructor. * * This makes disposal more costly, but has certain benefits: * - If anything still refers to the object and uses it, we'll get an early error, rather than @@ -139,36 +162,7 @@ export abstract class Disposable implements IDisposable { * which are numerous and short-lived (and less likely to be referenced from unexpected places). */ public wipeOnDispose(): void { - this.autoDisposeWith(_wipeOutObject, this); - } - - /** - * Remove `obj` from the list of owned objects; it will not be disposed on `this.dispose`. - * @param {Object} obj: Object to release. - * @returns {Object} obj - */ - public disposeRelease(obj: T): T { - const list = this._disposalList; - const index = list.findIndex((entry) => (entry.obj === obj)); - if (index !== -1) { - list.splice(index, 1); - } - return obj; - } - - /** - * Dispose an owned object `obj` now, and remove it from the list of owned objects. - * @param {Object} obj: Object to release. - * @returns nothing - */ - public disposeDiscard(obj: IDisposable) { - const list = this._disposalList; - const index = list.findIndex((entry) => (entry.obj === obj)); - if (index !== -1) { - const entry = list[index]; - list.splice(index, 1); - entry.disposer.call(this, obj); - } + this.onDispose(this._wipeOutObject, this); } /** @@ -179,65 +173,77 @@ export abstract class Disposable implements IDisposable { } /** - * Clean up `this` by disposing all owned objects, and calling `stopListening()` if defined. + * Clean up `this` by disposing all owned objects, and calling onDispose() callbacks, in reverse + * order to that in which they were added. */ public dispose(): void { - const list = this._disposalList; - if (list) { - // This makes isDisposed() true, and the object is no longer valid (in particular, - // this._disposalList no longer satisfies its declared type). - (this._disposalList as any) = null; - - // Go backwards through the disposal list, and dispose everything. - for (let i = list.length - 1; i >= 0; i--) { - const entry: IDisposalEntry = list[i]; - _disposeHelper(this, entry.disposer, entry.obj); - } - } + const disposalList = this._disposalList; + this._disposalList = null!; + disposalList.callAndDispose(this); } /** - * Called during construction. Implement this in subclasses to do constructor work safely. If - * this throws an exception, the partially-constructed object will get cleaned up -- i.e. any - * calls to `this.autoDispose()` that happened before the exception will be honored. - * Method create() is NOT intended to be called directly. + * Wipe out this object by setting each property to null. This is helpful for objects that are + * disposed and should be ready to be garbage-collected. */ - protected abstract create(...args: any[]): void; -} - -/** - * Internal helper to allow adding cleanup callbacks to the disposalList. It acts as the - * "disposer" for callback, by simply calling them with the same context that it is called with. - */ -function _callFuncHelper(this: Disposable, callback: () => void): void { - callback.call(this); + private _wipeOutObject(): void { + // The sentinel value doesn't have to be null, but some values cause more helpful errors than + // others. E.g. if a.x = "disposed", then a.x.foo() throws "undefined is not a function", but + // when a.x = null, a.x.foo() throws a more helpful "Cannot read property 'foo' of null". + for (const k of Object.keys(this)) { + (this as any)[k] = null; + } + } } /** - * Wipe out the given object by setting each property to a dummy sentinel value. This is helpful - * for objects that are disposed and should be ready to be garbage-collected. + * Holder keeps a single disposable object. If given responsibility for another object using + * holder.autoDispose() or Foo.create(holder, ...), it automatically disposes the currently held + * object. It also disposes it when the holder itself is disposed. * - * The sentinel value doesn't have to be null, but some values cause more helpful errors than - * others. E.g. if a.x = "disposed", then a.x.foo() throws "undefined is not a function", while - * when a.x = null, a.x.foo() throws "Cannot read property 'foo' of null", which is more helpful. + * TODO Holder needs unittests. */ -function _wipeOutObject(obj: {[key: string]: any}) { - Object.keys(obj).forEach((k) => (obj[k] = null)); +export class Holder implements IDisposable, IDisposableOwner { + public static create(owner: IDisposableOwner|null): Holder { + return _autoDispose(owner, new Holder()); + } + + private _owned: IDisposable|null = null; + + /** Take ownership of a new object, disposing the previously held one. */ + public autoDispose(obj: T): T { + if (this._owned) { this._owned.dispose(); } + this._owned = obj; + return obj; + } + + /** Releases the held object without disposing it, emptying the holder. */ + public release(): IDisposable|null { + const ret = this._owned; + this._owned = null; + return ret; + } + + /** Disposes the held object and empties the holder. */ + public clear(): void { + if (this._owned) { + this._owned.dispose(); + this._owned = null; + } + } + + /** When the holder is disposed, it disposes the held object if any. */ + public dispose(): void { + this.clear(); + } } /** - * Internal helper to call a disposer on an object. It swallows errors (but reports them) to make - * sure that when we dispose an object, an error in disposing one owned part doesn't stop - * the disposal of the other parts. + * Helper for more concise implementations of the `create(owner)` interface. */ -function _disposeHelper(owner: Disposable, disposer: (obj: any) => void, obj: any) { - try { - disposer.call(owner, obj); - } catch (e) { - // tslint:disable-next-line:no-console - console.error("While disposing %s, error disposing %s: %s", - _describe(owner), _describe(obj), e); - } +function _autoDispose(owner: IDisposableOwner|null, obj: T): T { + if (owner) { owner.autoDispose(obj); } + return obj; } /** @@ -248,8 +254,47 @@ function _describe(obj: any) { } /** - * Helper disposer that simply invokes the .dispose() method. + * DisposalList is an internal class mimicking emit.Emitter. The difference is that callbacks are + * called in reverse order, and exceptions in callbacks are reported and swallowed. */ -function _defaultDisposer(obj: IDisposable) { - obj.dispose(); +class DisposalList extends LLink { + constructor() { super(); } + + public addListener(callback: (this: T) => void, optContext?: T): void { + const lis = new DisposeListener(callback, optContext); + this._insertBefore(this._next!, lis); + } + + /** + * Call all callbacks and dispose this object. The owner is required for better reporting of + * errors if any callback throws. + */ + public callAndDispose(owner: Disposable): void { + try { + DisposeListener.callAll(this._next!, this, owner); + } finally { + this._disposeList(); + } + } +} + +/** + * Internal class that keeps track of one item of the DisposalList. It mimicks emit.Listener, but + * reports and swallows erros when it calls the callbacks in the list. + */ +class DisposeListener extends LLink { + public static callAll(begin: LLink, end: LLink, owner: Disposable): void { + while (begin !== end) { + const lis = begin as DisposeListener; + try { + lis.callback.call(lis.context); + } catch (e) { + // tslint:disable-next-line:no-console + console.error("While disposing %s, error disposing %s: %s", _describe(owner), _describe(this), e); + } + begin = lis._next!; + } + } + + constructor(private callback: () => void, private context?: any) { super(); } } diff --git a/lib/domevent.ts b/lib/domevent.ts index 1140816..d31e78a 100644 --- a/lib/domevent.ts +++ b/lib/domevent.ts @@ -20,10 +20,10 @@ * * listener.dispose(); * - * Disposing the listener returned by .on() is the only way to stop listening to an event. You can - * use autoDispose to stop listening automatically when subscribing in a Disposable object: + * Disposing the listener returned by .onElem() is the only way to stop listening to an event. You + * can use autoDispose to stop listening automatically when subscribing in a Disposable object: * - * this.autoDispose(domevent.on(document, 'mouseup', callback)); + * this.autoDispose(domevent.onElem(document, 'mouseup', callback)); * * To listen to descendants of an element matching the given selector (what JQuery calls * "delegated events", see http://api.jquery.com/on/): diff --git a/lib/emit.ts b/lib/emit.ts index 70fcf4b..b5a4c4f 100644 --- a/lib/emit.ts +++ b/lib/emit.ts @@ -38,7 +38,7 @@ function _noop() { /* noop */} -export type ListenerCB = (...args: any[]) => void; +export type ListenerCB = (this: T, ...args: any[]) => void; export type ChangeCB = (hasListeners: boolean) => void; /** @@ -101,7 +101,7 @@ export class Emitter extends LLink { * @param {Object} optContext: Context for the function. * @returns {Listener} Listener object. Its dispose() method removes the callback from the list. */ - public addListener(callback: ListenerCB, optContext?: object): Listener { + public addListener(callback: ListenerCB, optContext?: T): Listener { return new Listener(this, callback, optContext); } @@ -162,8 +162,8 @@ export class Listener extends LLink { } constructor(private emitter: Emitter, - private callback: ListenerCB, - private context?: object) { + private callback: ListenerCB, + private context?: any) { super(); this._insertBefore(emitter, this); emitter._triggerChangeCB(); diff --git a/package.json b/package.json index d6f5cc6..ec3e30c 100644 --- a/package.json +++ b/package.json @@ -8,7 +8,8 @@ "scripts": { "build": "bash build.sh", "watch": "tsc -w", - "test": "nyc mocha -R list -gc test/", + "test": "nyc mocha -R list test/", + "test-memory": "mocha -R list -gc test/", "test-timing": "TIMING_TESTS=1 mocha -R list test/", "build-demo": "browserify demo/celsius_grain/index.js -d | uglifyjs --source-map 'content=inline,url=build-index.js.map' -o demo/celsius_grain/build-index.js", "preversion": "npm test", diff --git a/test/dispose.js b/test/dispose.js index b3022e1..942ca24 100644 --- a/test/dispose.js +++ b/test/dispose.js @@ -25,11 +25,12 @@ describe('dispose', function() { let cleanup2 = sinon.spy(); class Foo extends Disposable { - create() { + constructor() { + super(); this.bar = this.autoDispose(bar); - this.baz = this.autoDisposeWithMethod('destroy', baz); - this.baz2 = this.autoDisposeWith(disposer, baz2); - this.autoDisposeCallback(cleanup1); + this.baz = this.onDispose(baz.destroy, baz); + this.baz2 = this.onDispose(() => disposer(baz2)); + this.onDispose(cleanup1, this); } } @@ -40,7 +41,7 @@ describe('dispose', function() { assert.equal(Object.getPrototypeOf(Foo).name, "Disposable"); // We can also use disposal methods outside the constructor. - foo.autoDisposeCallback(cleanup2); + foo.onDispose(cleanup2, foo); foo.dispose(); assert(foo.isDisposed()); @@ -55,7 +56,6 @@ describe('dispose', function() { assert(baz.destroy.calledWithExactly()); assert.equal(disposer.callCount, 1); - assert(disposer.calledOn(foo)); assert(disposer.calledWithExactly(baz2)); assert.equal(cleanup1.callCount, 1); @@ -74,16 +74,20 @@ describe('dispose', function() { it("should wipe object, only when requested", function() { let bar1 = new Bar(); - let bar2 = new Bar(); + let barA = new Bar(); + let barB = new Bar(); class Foo extends Disposable { - create() { + constructor() { + super(); this.bar = this.autoDispose(bar1); } } class FooWiped extends Disposable { - create() { + constructor() { + super(); + this.barA = this.autoDispose(barA); this.wipeOnDispose(); - this.bar = this.autoDispose(bar2); + this.barB = this.autoDispose(barB); } } @@ -94,9 +98,11 @@ describe('dispose', function() { assert(foo1.isDisposed()); assert(foo2.isDisposed()); assert.strictEqual(foo1.bar, bar1); - assert.strictEqual(foo2.bar, null); + assert.strictEqual(foo2.barA, null); + assert.strictEqual(foo2.barB, null); assert.equal(bar1.dispose.callCount, 1); - assert.equal(bar2.dispose.callCount, 1); + assert.equal(barA.dispose.callCount, 1); + assert.equal(barB.dispose.callCount, 1); }); }); @@ -105,7 +111,8 @@ describe('dispose', function() { let baz = new Bar(); class Foo extends Disposable { - create(throwWhen) { + constructor(throwWhen) { + super(); if (throwWhen === 0) { throw new Error("test-error"); } this.bar = this.autoDispose(bar); if (throwWhen === 1) { throw new Error("test-error"); } @@ -122,20 +129,20 @@ describe('dispose', function() { it("should dispose partially constructed objects", function() { let foo; // If we throw right away, no surprises, nothing gets called. - assert.throws(function() { foo = new Foo(0); }, /test-error/); + assert.throws(function() { foo = Foo.create(null, 0); }, /test-error/); assert.strictEqual(foo, undefined); assert.equal(bar.dispose.callCount, 0); assert.equal(baz.dispose.callCount, 0); // If we constructed one object, that one object should have gotten disposed. - assert.throws(function() { foo = new Foo(1); }, /test-error/); + assert.throws(function() { foo = Foo.create(null, 1); }, /test-error/); assert.strictEqual(foo, undefined); assert.equal(bar.dispose.callCount, 1); assert.equal(baz.dispose.callCount, 0); bar.dispose.resetHistory(); // If we constructed two objects, both should have gotten disposed. - assert.throws(function() { foo = new Foo(2); }, /test-error/); + assert.throws(function() { foo = Foo.create(null, 2); }, /test-error/); assert.strictEqual(foo, undefined); assert.equal(bar.dispose.callCount, 1); assert.equal(baz.dispose.callCount, 1); @@ -144,7 +151,7 @@ describe('dispose', function() { baz.dispose.resetHistory(); // If we don't throw, then nothing should get disposed until we call .dispose(). - assert.doesNotThrow(function() { foo = new Foo(3); }); + assert.doesNotThrow(function() { foo = Foo.create(null, 3); }); assert(!foo.isDisposed()); assert.equal(bar.dispose.callCount, 0); assert.equal(baz.dispose.callCount, 0); @@ -158,13 +165,14 @@ describe('dispose', function() { it("should dispose on propagated errors", function() { let bar2 = new Bar(); class Foo2 extends Disposable { - create() { + constructor() { + super(); this.bar2 = this.autoDispose(bar2); - this.foo = this.autoDispose(new Foo(1)); + this.foo = Foo.create(this, 1); } } let foo2; - assert.throws(function() { foo2 = new Foo2(); }, /test-error/); + assert.throws(function() { foo2 = Foo2.create(null); }, /test-error/); assert.strictEqual(foo2, undefined); assert.equal(bar.dispose.callCount, 1); assert.equal(baz.dispose.callCount, 0); @@ -172,15 +180,16 @@ describe('dispose', function() { }); class FailOnDispose extends Disposable { - create() { + constructor() { + super(); this.bar = this.autoDispose(bar); - this.foo = this.autoDisposeCallback(() => { throw new Error("test-error-disposal"); }); + this.foo = this.onDispose(() => { throw new Error("test-error-disposal"); }); } } it("should catch but report exceptions during disposal", function() { consoleCapture(['error'], messages => { - let fod = new FailOnDispose(); + let fod = FailOnDispose.create(null); assert.equal(bar.dispose.callCount, 0); assert.doesNotThrow(() => fod.dispose()); assert.equal(bar.dispose.callCount, 1); @@ -192,8 +201,9 @@ describe('dispose', function() { it("should be helpful on exceptions cleaning partially-constructed objects", function() { class FailOnConstruct extends Disposable { - create() { - this.fod = this.autoDispose(new FailOnDispose()); + constructor() { + super(); + this.fod = FailOnDispose.create(this); throw new Error("test-error-construct"); } dispose() { @@ -203,7 +213,7 @@ describe('dispose', function() { } consoleCapture(['error'], messages => { assert.equal(bar.dispose.callCount, 0); - assert.throws(() => new FailOnConstruct(), /test-error-construct/); + assert.throws(() => FailOnConstruct.create(null), /test-error-construct/); assert.equal(bar.dispose.callCount, 1); assert.deepEqual(messages, [ "error: While disposing FailOnDispose, error disposing Function: Error: test-error-disposal", diff --git a/test/dom.js b/test/dom.js index 9a8ed5c..e02abb7 100644 --- a/test/dom.js +++ b/test/dom.js @@ -346,7 +346,7 @@ describe('dom', function() { class Comp extends dom.Component { render(arg, spies) { spies.onConstruct(arg); - this.autoDisposeCallback(() => spies.onDispose()); + this.onDispose(() => spies.onDispose()); this.fakeAttr = "fakeAttr"; // This should NOT become an attribute. return dom('div', dom.toggleClass(arg.toUpperCase(), true), spies.onRender, @@ -423,12 +423,11 @@ describe('dom', function() { // Simulate the explanatory example in the comment to createElem() to show the problem, and // compare with the previous test case. The approach here is to create a class that can be // separately created and inserted into DOM by reusing the code for a real Component. So we - // override create() to call Comp's render but do nothing else. Then mount() finishes the - // work that Component's own create() normally does. + // override _mount() to do nothing, and call the original _mount() separately. class Comp2 extends Comp { - create(...args) { this._content = super.render(...args); } - render() { return this._content; } - mount(elem) { super.create(elem); } + constructor(...args) { super(null, ...args); } + _mount(elem, content) { this._content = content; } + mount(elem) { super._mount(elem, this._content); } } function _insert_(component) { return elem => component.mount(elem); @@ -497,7 +496,7 @@ describe('dom', function() { class Comp extends dom.Component { render(arg, spies) { spies.onConstruct(arg); - this.autoDisposeCallback(() => spies.onDispose()); + this.onDispose(() => spies.onDispose()); components.push(this); return [ dom('div', '(', dom.text(arg.toLowerCase()), ')'), From 469216d6c56b6f1a96c3cc076227bde92c2a3001 Mon Sep 17 00:00:00 2001 From: Dmitry S Date: Mon, 5 Feb 2018 01:10:06 -0500 Subject: [PATCH 2/3] Generalize and expose dom.replaceContent method, and add a test case for it --- lib/_domMethods.ts | 20 +++++++++++++------- test/dom.js | 29 +++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 7 deletions(-) diff --git a/lib/_domMethods.ts b/lib/_domMethods.ts index aca14cc..3dea4b2 100644 --- a/lib/_domMethods.ts +++ b/lib/_domMethods.ts @@ -207,17 +207,23 @@ export function getData(elem: Node, key: string) { return obj && obj[key]; } -// Helper for domComputed(); replace content between markerPre and markerPost with the given DOM -// content, running disposers if any on the removed content. -function _replaceContent(elem: Node, markerPre: Node, markerPost: Node, content: DomArg): void { - if (markerPre.parentNode === elem) { +/** + * Replaces the content between nodeBefore and nodeAfter, which should be two siblings within the + * same parent node. New content may be anything allowed as an argument to dom(), including null + * to insert nothing. Runs disposers, if any, on all removed content. + */ +export function replaceContent(nodeBefore: Node, nodeAfter: Node, content: DomArg): void { + const elem = nodeBefore.parentNode; + if (elem) { let next; - for (let n = markerPre.nextSibling; n && n !== markerPost; n = next) { + for (let n = nodeBefore.nextSibling; n && n !== nodeAfter; n = next) { next = n.nextSibling; domDispose(n); elem.removeChild(n); } - elem.insertBefore(frag(content), markerPost); + if (content) { + elem.insertBefore(content instanceof G.Node ? content : frag(content), nodeAfter); + } } } @@ -269,7 +275,7 @@ export function domComputed(valueObs: BindableValue, contentFunc?: (val: T elem.appendChild(markerPre); elem.appendChild(markerPost); _subscribe(elem, valueObs, - (value) => _replaceContent(elem, markerPre, markerPost, _contentFunc(value))); + (value) => replaceContent(markerPre, markerPost, _contentFunc(value))); }; } diff --git a/test/dom.js b/test/dom.js index e02abb7..8e1fb54 100644 --- a/test/dom.js +++ b/test/dom.js @@ -542,6 +542,35 @@ describe('dom', function() { }); }); + describe('replaceContent', function() { + it('should replace and dispose old content', function() { + let a, b; + let spy = sinon.spy(), spyOuter = sinon.spy(); + let elem = dom('div', a = dom('span', 'a'), b = dom('span', 'b'), dom.onDispose(spyOuter)); + assert.equal(elem.innerHTML, 'ab'); + dom.replaceContent(a, b, ['x', 'y', dom('span', dom.onDispose(spy), 'z')]); + assert.equal(elem.innerHTML, 'axyzb'); + + sinon.assert.notCalled(spy); + dom.replaceContent(a, b, dom('p', dom.onDispose(spy), 'X')); + assertResetSingleCall(spy, undefined, sinon.match.has('tagName', 'SPAN')); + assert.equal(elem.innerHTML, 'a

X

b'); + + sinon.assert.notCalled(spy); + dom.replaceContent(a, b, null); + assertResetSingleCall(spy, undefined, sinon.match.has('tagName', 'P')); + assert.equal(elem.innerHTML, 'ab'); + + dom.replaceContent(a, b, dom.frag('x', 'y', dom('br', dom.onDispose(spy)))); + assert.equal(elem.innerHTML, 'axy
b'); + + sinon.assert.notCalled(spy); + dom.domDispose(elem); + assert(spy.calledBefore(spyOuter)); + assertResetSingleCall(spy, undefined, sinon.match.has('tagName', 'BR')); + assertResetSingleCall(spyOuter, undefined, sinon.match.has('tagName', 'DIV')); + }); + }); describe('computed', function() { From 1d433ea559ed3011994b3fa2a8792f043c83689c Mon Sep 17 00:00:00 2001 From: Dmitry S Date: Mon, 5 Feb 2018 02:39:46 -0500 Subject: [PATCH 3/3] Refactor domComponent interface; replace render() override with setContent() call --- demo/celsius_grain/index.js | 7 +- lib/_domComponent.ts | 146 ++++++++++++++++++++---------------- lib/dom.ts | 2 +- test/dom.js | 55 ++++++-------- 4 files changed, 109 insertions(+), 101 deletions(-) diff --git a/demo/celsius_grain/index.js b/demo/celsius_grain/index.js index 6a6ae59..8ecfe82 100644 --- a/demo/celsius_grain/index.js +++ b/demo/celsius_grain/index.js @@ -35,7 +35,8 @@ function TemperatureInput(temperature, scaleName) { } class Calculator extends dom.Component { - render() { + constructor() { + super(); this._temp = observable(''); this._scale = observable('c'); @@ -43,11 +44,11 @@ class Calculator extends dom.Component { const fahrenheit = this.autoDispose(this._makeScaleTemp('f', toFahrenheit)); const celsiusValue = this.autoDispose(computed(use => parseFloat(use(celsius)))); - return dom('div', + this.setContent(dom('div', TemperatureInput(celsius, 'Celsius'), TemperatureInput(fahrenheit, 'Fahrenheit'), BoilingVerdict(celsiusValue) - ); + )); } _makeScaleTemp(toScale, converter) { diff --git a/lib/_domComponent.ts b/lib/_domComponent.ts index e1c8543..129595b 100644 --- a/lib/_domComponent.ts +++ b/lib/_domComponent.ts @@ -3,106 +3,125 @@ * createElem() and create(). */ -import {domDispose, onDisposeElem} from './_domDispose'; -import {DomElementArg, DomElementMethod, update} from './_domImpl'; +import {onDisposeElem} from './_domDispose'; +import {DomElementMethod, update} from './_domImpl'; +import {replaceContent} from './_domMethods'; import {Disposable, IDisposableOwner} from './dispose'; // Use the browser globals in a way that allows replacing them with mocks in tests. import {G} from './browserGlobals'; +// Static type of a class that inherits Component. +export interface IComponentClassType { + new (...args: any[]): T; + create(owner: Element|IDisposableOwner|null, ...args: any[]): T; +} + /** - * A UI component should extend this base class and implement `render()`. Compared to a simple - * function returning DOM (a "functional" component), a "class" component makes it easier to - * organize code into methods. + * Helper that takes ownership of a component by mounting it to a parent element. + */ +class DomOwner implements IDisposableOwner { + constructor(private _parentElem: Element) {} + public autoDispose(comp: Component): void { comp.mount(this._parentElem); } +} + +/** + * A UI component should extend this base class and implement a constructor that creates some DOM + * and calls this.setContent() with it. Compared to a simple function returning DOM (a + * "functional" component), a "class" component makes it easier to organize code into methods. * * In addition, a "class" component may be disposed to remove it from the DOM, although this is * uncommon since a UI component is normally owned by its containing DOM. */ export abstract class Component extends Disposable { - private _markerPre: Node|undefined; - private _markerPost: Node|undefined; - /** - * Component should implement render(). If overriding constructor, remember to pass along - * arguments, and keep in mind that render() will be called before additional constructor code. - * TODO This should have typescript overloads to ensure that it takes the same arguments as - * render(). + * Create a component using Foo.create(owner, ...args) similarly to creating any other + * Disposable object. The difference is that `owner` may be a DOM Element, and the content set + * by the constructor's setContent() call will be appended to and owned by that owner element. + * + * If the owner is not an Element, works like a regular Disposable. To add such a component to + * DOM, use the mount() method. */ - constructor(elem: Element, ...args: any[]) { - super(); - this._mount(elem, this.render(...args)); + // TODO add typescript overloads for strict argument checks. + public static create(this: IComponentClassType, + owner: Element|IDisposableOwner|null, ...args: any[]): T { + const _owner: IDisposableOwner|null = owner instanceof G.Element ? new DomOwner(owner) : owner; + return Disposable.create.call(this, _owner, ...args); } - /** - * Components must extend this class and implement a `render()` method, which is called at - * construction with constructor arguments, and should return DOM for the component. - * - * It is recommended that any constructor work is done in this method. - * - * render() may return any type of value that's accepted by dom() as an argument, including a - * DOM element, a string, null, or an array. The returned DOM is automatically owned by the - * component, so do not wrap it in `this.autoDispose()`. - */ - public abstract render(...args: any[]): DomElementArg; + private _markerPre: Node = G.document.createComment('A'); + private _markerPost: Node = G.document.createComment('B'); + private _contentToMount: Node|null = null; - /** - * Inserts the content into DOM, arranging for it to be disposed when this Component is, and to - * dispose the Component when the parent element gets disposed. - */ - private _mount(elem: Element, content: DomElementArg): void { - this._markerPre = G.document.createComment('A'); - this._markerPost = G.document.createComment('B'); + constructor() { + super(); // If the containing DOM is disposed, it will dispose all of our DOM (included among children // of the containing DOM). Let it also dispose this Component when it gets to _markerPost. // Since _unmount() is unnecessary here, we skip its work by unseting _markerPre/_markerPost. onDisposeElem(this._markerPost, () => { - this._markerPre = this._markerPost = undefined; + this._markerPre = this._markerPost = undefined!; this.dispose(); }); // When the component is disposed, unmount the DOM we created (i.e. dispose and remove). // Except that we skip this as unnecessary when the disposal is triggered by containing DOM. this.onDispose(this._unmount, this); + } + + /** + * Inserts the content of this component into a parent DOM element. + */ + public mount(elem: Element): void { + // Insert the result of setContent() into the given parent element. Note that mount() must + // only ever be called once. It is normally called as part of .create(). + if (!this._markerPost) { throw new Error('Component mount() called when already disposed'); } + if (this._markerPost.parentNode) { throw new Error('Component mount() called twice'); } + update(elem, this._markerPre, this._contentToMount, this._markerPost); + this._contentToMount = null; + } - // Insert the result of render() into the given parent element. - update(elem, this._markerPre, content, this._markerPost); + /** + * Components should call setContent() with their DOM content, typically in the constructor. If + * called outside the constructor, setContent() will replace previously set DOM. It accepts any + * DOM Node; use dom.frag() to insert multiple nodes together. + */ + protected setContent(content: Node): void { + if (this._markerPost) { + if (this._markerPost.parentNode) { + // Component is already mounted. Replace previous content. + replaceContent(this._markerPre!, this._markerPost, content); + } else { + // Component is created but not yet mounted. Save the content for the mount() call. + this._contentToMount = content; + } + } } /** - * Detaches and disposes the DOM created and attached in _mount(). + * Detaches and disposes the DOM created and attached in mount(). */ private _unmount(): void { // Dispose the owned content, and remove it from the DOM. The conditional skips the work when // the unmounting is triggered by the disposal of the containing DOM. - if (this._markerPre && this._markerPre.parentNode) { - let next; - const elem = this._markerPre.parentNode; - for (let n = this._markerPre.nextSibling; n && n !== this._markerPost; n = next) { - next = n.nextSibling; - domDispose(n); - elem.removeChild(n); - } - elem.removeChild(this._markerPre); - elem.removeChild(this._markerPost!); + if (this._markerPost && this._markerPost.parentNode) { + const elem = this._markerPost.parentNode; + replaceContent(this._markerPre!, this._markerPost, null); + elem.removeChild(this._markerPre!); + elem.removeChild(this._markerPost); } + this._markerPre = this._markerPost = undefined!; } } -export interface IComponentClassType { - new (...args: any[]): T; - create(owner: IDisposableOwner|null, ...args: any[]): T; -} - /** * Construct and insert a UI component into the given DOM element. The component must extend - * dom.Component, and must implement a `render(...)` method which should do any rendering work - * work and return DOM. DOM may be any type value accepted by dom() as an argument, including a - * DOM element, string, null, or array. The returned DOM is automatically owned by the component. + * dom.Component, and should build DOM and call setContent(DOM) in the constructor. DOM may be any + * Node. Use dom.frag() to insert multiple nodes together. * - * Logically, the parent `elem` owns the created component, and the component owns the DOM - * returned by its render() method. If the parent is disposed, so is the component and its DOM. If - * the component is somehow disposed directly, then its DOM is disposed and removed from `elem`. + * Logically, the parent `elem` owns the created component, and the component owns the DOM set by + * setContent(). If the parent is disposed, so is the component and its DOM. If the component is + * somehow disposed directly, then its DOM is disposed and removed from `elem`. * * Note the correct usage: * @@ -120,14 +139,11 @@ export interface IComponentClassType { * @param {Element} elem: The element to which to append the newly constructed component. * @param {Class} ComponentClass: The component class to instantiate. It must extend * dom.Component(...) and implement the render() method. - * @param {Objects} ...args: Arguments to the constructor which passes them to the render method. + * @param {Objects} ...args: Arguments to the Component's constructor. */ -export function createElem(elem: Element, cls: IComponentClassType, ...args: any[]): T { - return cls.create(null, elem, ...args); -} +// TODO add typescript overloads for strict argument checks. export function create(cls: IComponentClassType, ...args: any[]): DomElementMethod { - // tslint:disable-next-line:no-unused-expression - return (elem) => { cls.create(null, elem, ...args); }; + return (elem) => { cls.create(elem, ...args); }; } /** @@ -145,7 +161,7 @@ export function create(cls: IComponentClassType, ...args export function createInit(cls: IComponentClassType, ...args: any[]): DomElementMethod { return (elem) => { const initFunc: (c: T) => void = args.pop(); - const c = cls.create(null, elem, ...args); + const c = cls.create(elem, ...args); initFunc(c); }; } diff --git a/lib/dom.ts b/lib/dom.ts index 9998806..83506c1 100644 --- a/lib/dom.ts +++ b/lib/dom.ts @@ -73,11 +73,11 @@ export namespace dom { // tslint:disable-line:no-namespace export const dataElem = _domMethods.dataElem; export const data = _domMethods.data; export const getData = _domMethods.getData; + export const replaceContent = _domMethods.replaceContent; export const domComputed = _domMethods.domComputed; export const maybe = _domMethods.maybe; export const Component = _domComponent.Component; - export const createElem = _domComponent.createElem; export const create = _domComponent.create; export const createInit = _domComponent.createInit; diff --git a/test/dom.js b/test/dom.js index 8e1fb54..5c8a20e 100644 --- a/test/dom.js +++ b/test/dom.js @@ -344,13 +344,14 @@ describe('dom', function() { describe("component", function() { class Comp extends dom.Component { - render(arg, spies) { + constructor(arg, spies) { + super(); spies.onConstruct(arg); this.onDispose(() => spies.onDispose()); this.fakeAttr = "fakeAttr"; // This should NOT become an attribute. - return dom('div', dom.toggleClass(arg.toUpperCase(), true), + this.setContent(dom('div', dom.toggleClass(arg.toUpperCase(), true), spies.onRender, - dom.onDispose(spies.onDomDispose)); + dom.onDispose(spies.onDomDispose))); } } @@ -363,7 +364,7 @@ describe('dom', function() { }; } - it("should call render and disposers correctly", function() { + it("should use content and call disposers correctly", function() { let spies1 = makeSpies(), spies2 = makeSpies(); let elem = dom('div', 'Hello', @@ -420,15 +421,8 @@ describe('dom', function() { let spies1 = makeSpies(), spies2 = makeSpies(); spies2.onConstruct = sinon.stub().throws(new Error('ctor throw')); - // Simulate the explanatory example in the comment to createElem() to show the problem, and - // compare with the previous test case. The approach here is to create a class that can be - // separately created and inserted into DOM by reusing the code for a real Component. So we - // override _mount() to do nothing, and call the original _mount() separately. - class Comp2 extends Comp { - constructor(...args) { super(null, ...args); } - _mount(elem, content) { this._content = content; } - mount(elem) { super._mount(elem, this._content); } - } + // Simulate the explanatory example in the comment to dom.create() to show the problem, and + // compare with the previous test case. function _insert_(component) { return elem => component.mount(elem); } @@ -436,8 +430,8 @@ describe('dom', function() { consoleCapture(['error'], messages => { assert.throws(() => dom('div', 'Hello', - _insert_(new Comp2('foo', spies1)), - _insert_(new Comp2('bar', spies2)), + _insert_(new Comp('foo', spies1)), + _insert_(new Comp('bar', spies2)), 'World' ), /ctor throw/ @@ -491,21 +485,20 @@ describe('dom', function() { assertResetSingleCall(spies2.onDomDispose, undefined, sinon.match.has('className', 'BAR')); }); - it('should support render() returning any number of children', function() { + it('should support setContent() with a doc fragment', function() { let components = []; class Comp extends dom.Component { - render(arg, spies) { + constructor(arg, spies) { + super(); spies.onConstruct(arg); this.onDispose(() => spies.onDispose()); components.push(this); - return [ - dom('div', '(', dom.text(arg.toLowerCase()), ')'), + this.setContent(dom.frag( + dom('div', '(', dom.text(arg.toLowerCase()), ')', + dom.onDispose(spies.onDomDispose)), dom('span', '[', dom.text(arg.toUpperCase()), ']'), - spies.onRender, - // A disposer like this is only run when the DOM containing this Component is - // disposed, it doesn't get run when the component itself is disposed. - dom.onDispose(spies.onDomDispose) - ]; + spies.onRender + )); } } @@ -518,8 +511,8 @@ describe('dom', function() { assertResetSingleCall(spies1.onConstruct, spies1, 'Xx'); assertResetSingleCall(spies2.onConstruct, spies2, 'Yy'); - assertResetSingleCall(spies1.onRender, undefined, sinon.match.same(elem)); - assertResetSingleCall(spies2.onRender, undefined, sinon.match.same(elem)); + assertResetSingleCall(spies1.onRender, undefined, sinon.match.instanceOf(G.DocumentFragment)); + assertResetSingleCall(spies2.onRender, undefined, sinon.match.instanceOf(G.DocumentFragment)); assert.equal(elem.innerHTML, 'Hello
(xx)
[XX]' + '
(yy)
[YY]World'); @@ -528,17 +521,15 @@ describe('dom', function() { components[0].dispose(); assert.equal(elem.innerHTML, 'Hello
(yy)
[YY]World'); assertResetSingleCall(spies1.onDispose, spies1); - // This isn't a great behavior, but it makes sense: a disposer returned as part of an array - // is attached to the parent containing the Component, and not called until THAT's disposed. - sinon.assert.notCalled(spies1.onDomDispose); + assertResetSingleCall(spies1.onDomDispose, undefined, sinon.match.has('tagName', 'DIV')); + sinon.assert.notCalled(spies2.onDomDispose); // If we dispose the parent, the DOM doesn't need to be modified, but disposers get called. dom.domDispose(elem); // Check that DOM isn't modified (that would be wasteful). assert.equal(elem.innerHTML, 'Hello
(yy)
[YY]World'); - assertResetSingleCall(spies2.onDispose, spies2); - assertResetSingleCall(spies2.onDomDispose, undefined, sinon.match.same(elem)); - assertResetSingleCall(spies1.onDomDispose, undefined, sinon.match.same(elem)); + assertResetSingleCall(spies2.onDomDispose, undefined, sinon.match.has('tagName', 'DIV')); + sinon.assert.notCalled(spies1.onDomDispose); }); });