From f79e6a88e6608d2d679d6ea1b86b63e9f14e8336 Mon Sep 17 00:00:00 2001 From: Francois Chabot Date: Fri, 23 Apr 2021 08:19:05 -0400 Subject: [PATCH 1/5] removed redundant loop --- src/Node.ts | 4 ++-- src/Prober.ts | 21 ++++++--------------- 2 files changed, 8 insertions(+), 17 deletions(-) diff --git a/src/Node.ts b/src/Node.ts index 2eab458..baec1d1 100644 --- a/src/Node.ts +++ b/src/Node.ts @@ -25,8 +25,8 @@ export interface NodeBuildData { _cb: (...arg: unknown[]) => unknown; _args: unknown[]; - _next?: IPNode; - _resolveAs?: IPNode; + _next?: BaseNode; + _resolveAs: BaseNode; _prober: IProber; _context: ProbingContext; } diff --git a/src/Prober.ts b/src/Prober.ts index 7fe0a0d..6e25b56 100644 --- a/src/Prober.ts +++ b/src/Prober.ts @@ -87,6 +87,7 @@ class Prober implements IProber { } newNode._buildData = { + _resolveAs: newNode, _cb, _prober: this, _args, @@ -100,10 +101,7 @@ class Prober implements IProber { _finalizeNode(node: BaseNode) { // If a component returns a Node (as opposed to a value), then we short-circuit to the parent. - let destinationNode = node; - while (destinationNode._buildData && destinationNode._buildData._resolveAs) { - destinationNode = destinationNode._buildData!._resolveAs; - } + const destinationNode = node._buildData!._resolveAs; this._current._node = node; const { _cb, _args } = node._buildData!; @@ -115,7 +113,7 @@ class Prober implements IProber { destinationNode._result = cbResult._result; if (cbResult._onDispose) { addToDisposeQueue(destinationNode, cbResult._onDispose); - cbResult._onDispose = []; + cbResult._onDispose = undefined; } } else { cbResult._buildData!._resolveAs = destinationNode; @@ -144,8 +142,8 @@ class Prober implements IProber { } } - let node: BaseNode = this._current._announced!._head!; - let end: BaseNode = target as BaseNode; + let node = this._current._announced!._head!; + let end = target as BaseNode; if (end._buildData!._next) { this._current._announced!._head = end._buildData!._next; @@ -154,13 +152,6 @@ class Prober implements IProber { } end._buildData!._next = undefined; - /* - //These two steps are, I suspect, Technically unnecessary - - if (!this._current._announced._head) { - this._current._announced = {}; - } - */ pushEnv(this); this._stack.push(this._current); this._current = { _node: node, _disposeOps: [] }; @@ -177,7 +168,7 @@ class Prober implements IProber { } done = node === end; - const nextNode = node._buildData!._next as BaseNode; + const nextNode = node._buildData!._next; node._buildData = undefined; node = nextNode; } From 0ff0954d98c4f0e98243efbad012b8eddb282833 Mon Sep 17 00:00:00 2001 From: Francois Chabot Date: Fri, 23 Apr 2021 08:24:59 -0400 Subject: [PATCH 2/5] dispose queue manips belong to the node --- src/Node.ts | 8 ++++++++ src/Prober.ts | 14 +++----------- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/Node.ts b/src/Node.ts index baec1d1..d892aa6 100644 --- a/src/Node.ts +++ b/src/Node.ts @@ -49,6 +49,14 @@ export abstract class BaseNode implements IPNode { } } + _addToDispose(ops: DisposeOp[]): void { + if (!this._onDispose) { + this._onDispose = ops; + } else { + this._onDispose = this._onDispose.concat(ops); + } + }; + abstract get result(): unknown; _result?: unknown; diff --git a/src/Prober.ts b/src/Prober.ts index 6e25b56..b45aef3 100644 --- a/src/Prober.ts +++ b/src/Prober.ts @@ -20,14 +20,6 @@ const isIntrinsic = (cb: IKeys | ((...args: unknown[]) => unknown)): cb is return typeof cb === 'string'; }; -const addToDisposeQueue = (node: BaseNode, ops: DisposeOp[]) => { - if (!node._onDispose) { - node._onDispose = ops; - } else { - node._onDispose = node._onDispose.concat(ops); - } -}; - let _NextUniqueNodeId = 0; interface NodeQueue { @@ -112,7 +104,7 @@ class Prober implements IProber { // Post-ex-facto proxying. destinationNode._result = cbResult._result; if (cbResult._onDispose) { - addToDisposeQueue(destinationNode, cbResult._onDispose); + destinationNode._addToDispose(cbResult._onDispose); cbResult._onDispose = undefined; } } else { @@ -123,7 +115,7 @@ class Prober implements IProber { } if (this._current._disposeOps.length > 0) { - addToDisposeQueue(destinationNode, this._current._disposeOps); + destinationNode._addToDispose(this._current._disposeOps); this._current._disposeOps = []; } } @@ -170,7 +162,7 @@ class Prober implements IProber { done = node === end; const nextNode = node._buildData!._next; node._buildData = undefined; - node = nextNode; + node = nextNode!; } this._current = this._stack.pop()!; From e576ca1bdc74be44a4c1908dd03516e852b8c1d0 Mon Sep 17 00:00:00 2001 From: Francois Chabot Date: Fri, 23 Apr 2021 09:46:08 -0400 Subject: [PATCH 3/5] Less conditional state --- src/Environment.ts | 9 ++--- src/Node.ts | 16 +++------ src/Prober.ts | 62 ++++++++++++++++----------------- tests/dynamicList.test.ts | 2 +- tests/dynamicOperations.test.ts | 2 +- tests/dynamicVal.test.ts | 2 +- tests/environment.test.ts | 4 +-- 7 files changed, 43 insertions(+), 54 deletions(-) diff --git a/src/Environment.ts b/src/Environment.ts index 41509cc..3b11080 100644 --- a/src/Environment.ts +++ b/src/Environment.ts @@ -19,7 +19,7 @@ import { ProbingContext } from './ApiTypes'; export type DisposeOp = () => void; export interface Environment { _onDispose: (op: DisposeOp) => void; - _getProbingContext: () => ProbingContext | undefined; + _getProbingContext: () => ProbingContext; } let _currentEnv: Environment | null = null; @@ -50,10 +50,5 @@ export function useProbingContext(): ProbingContext { throw new Error('Environment underflow'); } - const result = _currentEnv!._getProbingContext(); - if (process.env.NODE_ENV !== 'production' && !result) { - throw new Error('There is no active probing context'); - } - - return result!; + return _currentEnv!._getProbingContext(); } diff --git a/src/Node.ts b/src/Node.ts index d892aa6..29919b8 100644 --- a/src/Node.ts +++ b/src/Node.ts @@ -43,25 +43,19 @@ export abstract class BaseNode implements IPNode { } dispose(): void { - if (this._onDispose) { - this._onDispose.forEach((c) => c()); - this._onDispose = undefined; - } + // Nodes should only ever be disposed once + this._onDispose.forEach((c) => c()); } _addToDispose(ops: DisposeOp[]): void { - if (!this._onDispose) { - this._onDispose = ops; - } else { - this._onDispose = this._onDispose.concat(ops); - } - }; + this._onDispose.push(...ops); + } abstract get result(): unknown; _result?: unknown; _probed_pnodetype?: number; - _onDispose?: DisposeOp[]; + _onDispose: DisposeOp[] = []; _buildData?: NodeBuildData; _uniqueNodeId?: number; diff --git a/src/Prober.ts b/src/Prober.ts index b45aef3..68d98fd 100644 --- a/src/Prober.ts +++ b/src/Prober.ts @@ -37,14 +37,14 @@ class Prober implements IProber { private _intrinsics: Partial; private _fallback?: IntrinsicFallback; private _stack: ProberStackFrame[] = []; - private _current: ProberStackFrame = { _disposeOps: [] }; + private _currentFrame: ProberStackFrame = { _disposeOps: [] }; _onDispose(op: DisposeOp): void { - this._current._disposeOps.push(op); + this._currentFrame._disposeOps.push(op); } - _getProbingContext(): ProbingContext | undefined { - return this._current!._node!._buildData!._context; + _getProbingContext(): ProbingContext { + return this._currentFrame!._node!._buildData!._context; } constructor(intrinsics: Partial, fallback?: IntrinsicFallback) { @@ -71,11 +71,11 @@ class Prober implements IProber { newNode._uniqueNodeId = _NextUniqueNodeId++; } - if (!this._current._announced) { - this._current._announced = { _head: newNode, _tail: newNode }; + if (!this._currentFrame._announced) { + this._currentFrame._announced = { _head: newNode, _tail: newNode }; } else { - this._current._announced._tail._buildData!._next = newNode; - this._current._announced._tail = newNode; + this._currentFrame._announced._tail._buildData!._next = newNode; + this._currentFrame._announced._tail = newNode; } newNode._buildData = { @@ -93,11 +93,11 @@ class Prober implements IProber { _finalizeNode(node: BaseNode) { // If a component returns a Node (as opposed to a value), then we short-circuit to the parent. - const destinationNode = node._buildData!._resolveAs; + const bd = node._buildData!; + const destinationNode = bd._resolveAs; - this._current._node = node; - const { _cb, _args } = node._buildData!; - const cbResult = _cb(..._args, node._buildData!._context); + this._currentFrame._node = node; + const cbResult = bd._cb(...bd._args, bd._context); if (isPNode(cbResult)) { if (cbResult.finalized) { @@ -114,17 +114,17 @@ class Prober implements IProber { destinationNode._result = cbResult; } - if (this._current._disposeOps.length > 0) { - destinationNode._addToDispose(this._current._disposeOps); - this._current._disposeOps = []; + if (this._currentFrame._disposeOps.length > 0) { + destinationNode._addToDispose(this._currentFrame._disposeOps); + this._currentFrame._disposeOps = []; } } _finalize(target: IPNode): void { if (process.env.NODE_ENV === 'check') { let lookup: BaseNode | undefined; - if (this._current._announced) { - lookup = this._current._announced._head; + if (this._currentFrame._announced) { + lookup = this._currentFrame._announced._head; } while (lookup && lookup !== target) { lookup = lookup._buildData!._next; @@ -134,29 +134,31 @@ class Prober implements IProber { } } - let node = this._current._announced!._head!; + let node = this._currentFrame._announced!._head!; let end = target as BaseNode; if (end._buildData!._next) { - this._current._announced!._head = end._buildData!._next; + this._currentFrame._announced!._head = end._buildData!._next; } else { - this._current._announced = undefined; + this._currentFrame._announced = undefined; } end._buildData!._next = undefined; pushEnv(this); - this._stack.push(this._current); - this._current = { _node: node, _disposeOps: [] }; + this._stack.push(this._currentFrame); + this._currentFrame = { _node: node, _disposeOps: [] }; let done = false; while (!done) { this._finalizeNode(node); - // Queue up any work that was discovered in the process. - if (this._current._announced) { - end = this._current._announced._tail; - node._buildData!._next = this._current._announced._head; - this._current._announced = undefined; + // Queue up any work that was discovered in the process, + // and update our end target so that we complete it before + // returning. + if (this._currentFrame._announced) { + end = this._currentFrame._announced._tail; + node._buildData!._next = this._currentFrame._announced._head; + this._currentFrame._announced = undefined; } done = node === end; @@ -165,20 +167,18 @@ class Prober implements IProber { node = nextNode!; } - this._current = this._stack.pop()!; + this._currentFrame = this._stack.pop()!; popEnv(); } private _getCb | ComponentCb>(what: T): { _cb: ComponentCb; _name: string } { if (isIntrinsic(what)) { let _cb: ComponentCb | undefined = this._intrinsics[what]; - const _name = what.toString(); if (!_cb) { - // This is safe, it's caught at the start of _announce() _cb = this._fallback!; } - return { _cb: _cb!, _name }; + return { _cb: _cb!, _name: what.toString() }; } else { return { _cb: what as ComponentCb, _name: (what as ComponentCb).name }; } diff --git a/tests/dynamicList.test.ts b/tests/dynamicList.test.ts index 7792124..b258308 100644 --- a/tests/dynamicList.test.ts +++ b/tests/dynamicList.test.ts @@ -27,7 +27,7 @@ const cleanup = () => { beforeEach(() => { pushEnv({ _onDispose: (op: DisposeOp) => disposeQueue.push(op), - _getProbingContext: () => undefined, + _getProbingContext: () => ({ componentName: '' }), }); }); diff --git a/tests/dynamicOperations.test.ts b/tests/dynamicOperations.test.ts index f13f0ef..5893040 100644 --- a/tests/dynamicOperations.test.ts +++ b/tests/dynamicOperations.test.ts @@ -27,7 +27,7 @@ const cleanup = () => { beforeEach(() => { pushEnv({ _onDispose: (op: DisposeOp) => disposeQueue.push(op), - _getProbingContext: () => undefined, + _getProbingContext: () => ({ componentName: '' }), }); }); diff --git a/tests/dynamicVal.test.ts b/tests/dynamicVal.test.ts index 8294094..fb7c62f 100644 --- a/tests/dynamicVal.test.ts +++ b/tests/dynamicVal.test.ts @@ -26,7 +26,7 @@ const cleanup = () => { beforeEach(() => { pushEnv({ _onDispose: (op: DisposeOp) => disposeQueue.push(op), - _getProbingContext: () => undefined, + _getProbingContext: () => ({ componentName: '' }), }); }); diff --git a/tests/environment.test.ts b/tests/environment.test.ts index 6938e1c..05d3d9b 100644 --- a/tests/environment.test.ts +++ b/tests/environment.test.ts @@ -20,12 +20,12 @@ describe('useOnDispose ', () => { class TestEnv implements Environment { count = 0; - probeContext?: ProbingContext; + probeContext: ProbingContext = { componentName: '' }; _onDispose(): void { this.count += 1; } - _getProbingContext(): ProbingContext | undefined { + _getProbingContext(): ProbingContext { return this.probeContext; } } From b470d169ff5872111e354cf46a0395b8f31f78bf Mon Sep 17 00:00:00 2001 From: Francois Chabot Date: Fri, 23 Apr 2021 09:59:39 -0400 Subject: [PATCH 4/5] Removed deprecated/redudant checks --- src/Prober.ts | 2 +- tests/environment.test.ts | 7 ------- 2 files changed, 1 insertion(+), 8 deletions(-) diff --git a/src/Prober.ts b/src/Prober.ts index 68d98fd..0de5cf7 100644 --- a/src/Prober.ts +++ b/src/Prober.ts @@ -105,7 +105,7 @@ class Prober implements IProber { destinationNode._result = cbResult._result; if (cbResult._onDispose) { destinationNode._addToDispose(cbResult._onDispose); - cbResult._onDispose = undefined; + // No need to clear the list on cbResult, that node is getting dropped. } } else { cbResult._buildData!._resolveAs = destinationNode; diff --git a/tests/environment.test.ts b/tests/environment.test.ts index 05d3d9b..8ab16ba 100644 --- a/tests/environment.test.ts +++ b/tests/environment.test.ts @@ -77,13 +77,6 @@ describe('useProbingContext ', () => { expectThrowInNotProd(useProbingContext); }); - it('Fails if probing context is set', () => { - const env = new TestEnv(); - pushEnv(env); - expectThrowInNotProd(useProbingContext); - popEnv(); - }); - it('Works if a probing context is set', () => { const env = new TestEnv(); env.probeContext = { componentName: 'aaa' }; From a68d696ca37951f256f4ee2052e5dd09e53aab67 Mon Sep 17 00:00:00 2001 From: Francois Chabot Date: Fri, 23 Apr 2021 10:13:47 -0400 Subject: [PATCH 5/5] Delegate node reassignment to the node itself. --- src/Node.ts | 5 +++++ src/Prober.ts | 7 +------ 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/Node.ts b/src/Node.ts index 29919b8..ee8444d 100644 --- a/src/Node.ts +++ b/src/Node.ts @@ -51,6 +51,11 @@ export abstract class BaseNode implements IPNode { this._onDispose.push(...ops); } + _assign(rhs: BaseNode): void { + this._result = rhs._result; + this._onDispose.push(...rhs._onDispose); + } + abstract get result(): unknown; _result?: unknown; diff --git a/src/Prober.ts b/src/Prober.ts index 0de5cf7..4df51c6 100644 --- a/src/Prober.ts +++ b/src/Prober.ts @@ -101,12 +101,7 @@ class Prober implements IProber { if (isPNode(cbResult)) { if (cbResult.finalized) { - // Post-ex-facto proxying. - destinationNode._result = cbResult._result; - if (cbResult._onDispose) { - destinationNode._addToDispose(cbResult._onDispose); - // No need to clear the list on cbResult, that node is getting dropped. - } + destinationNode._assign(cbResult); } else { cbResult._buildData!._resolveAs = destinationNode; }