From 3daedfa2848439cfc17fad15dbdade3ea0270106 Mon Sep 17 00:00:00 2001 From: yz-yu Date: Mon, 21 Jan 2019 19:18:51 +0800 Subject: [PATCH] fix remove node observer and check on the result of getNode (#43) * check removed node and its parent before collect * add more more checks on the result of getNode --- src/record/observer.ts | 23 +++++----- src/replay/index.ts | 47 ++++++++++++--------- src/types.ts | 3 +- test/__snapshots__/integration.test.ts.snap | 4 -- 4 files changed, 41 insertions(+), 36 deletions(-) diff --git a/src/record/observer.ts b/src/record/observer.ts index c31125329f..7b9025520e 100644 --- a/src/record/observer.ts +++ b/src/record/observer.ts @@ -102,6 +102,8 @@ function initMutationObserver(cb: mutationCallBack): MutationObserver { case 'childList': { addedNodes.forEach(n => genAdds(n)); removedNodes.forEach(n => { + const nodeId = mirror.getId(n as INode); + const parentId = mirror.getId(target as INode); if (isBlocked(n)) { return; } @@ -109,17 +111,24 @@ function initMutationObserver(cb: mutationCallBack): MutationObserver { if (addsSet.has(n)) { addsSet.delete(n); dropped.push(n); - } else if (addsSet.has(target) && !mirror.getId(n as INode)) { + } else if (addsSet.has(target) && nodeId === -1) { /** * If target was newly added and removed child node was * not serialized, it means the child node has been removed * before callback fired, so we can ignore it. * TODO: verify this */ + } else if (!mirror.has(parentId)) { + /** + * If parent id was not in the mirror map any more, it + * means the parent node has already been removed. So + * the node is also removed which we do not need to track + * and replay. + */ } else { removes.push({ - parentId: mirror.getId(target as INode), - id: mirror.getId(n as INode), + parentId, + id: nodeId, }); } mirror.removeNodeFromMap(n as INode); @@ -131,14 +140,6 @@ function initMutationObserver(cb: mutationCallBack): MutationObserver { } }); - removes = removes.map(remove => { - if (remove.parentNode) { - remove.parentId = mirror.getId(remove.parentNode as INode); - delete remove.parentNode; - } - return remove; - }); - const isDropped = (n: Node): boolean => { const { parentNode } = n; if (!parentNode) { diff --git a/src/replay/index.ts b/src/replay/index.ts index 0faf6fe659..032f42c88a 100644 --- a/src/replay/index.ts +++ b/src/replay/index.ts @@ -328,11 +328,12 @@ export class Replayer { d.removes.forEach(mutation => { const target = mirror.getNode(mutation.id); if (!target) { - return this.warnTargetNotFound(d, mutation.id); + return this.warnNodeNotFound(d, mutation.id); + } + const parent = mirror.getNode(mutation.parentId); + if (!parent) { + return this.warnNodeNotFound(d, mutation.parentId); } - const parent = (mirror.getNode( - mutation.parentId!, - ) as Node) as Element; // target may be removed with its parents before mirror.removeNodeFromMap(target); if (parent) { @@ -348,7 +349,10 @@ export class Replayer { mirror.map, true, ) as Node; - const parent = (mirror.getNode(mutation.parentId) as Node) as Element; + const parent = mirror.getNode(mutation.parentId); + if (!parent) { + return this.warnNodeNotFound(d, mutation.parentId); + } let previous: Node | null = null; let next: Node | null = null; if (mutation.previousId) { @@ -387,18 +391,27 @@ export class Replayer { } d.texts.forEach(mutation => { - const target = (mirror.getNode(mutation.id) as Node) as Text; + const target = mirror.getNode(mutation.id); + if (!target) { + return this.warnNodeNotFound(d, mutation.id); + } target.textContent = mutation.value; }); d.attributes.forEach(mutation => { - const target = (mirror.getNode(mutation.id) as Node) as Element; + const target = mirror.getNode(mutation.id); + if (!target) { + return this.warnNodeNotFound(d, mutation.id); + } for (const attributeName in mutation.attributes) { if (typeof attributeName === 'string') { const value = mutation.attributes[attributeName]; if (value) { - target.setAttribute(attributeName, value); + ((target as Node) as Element).setAttribute( + attributeName, + value, + ); } else { - target.removeAttribute(attributeName); + ((target as Node) as Element).removeAttribute(attributeName); } } } @@ -415,7 +428,7 @@ export class Replayer { this.mouse.style.top = `${p.y}px`; const target = mirror.getNode(p.id); if (!target) { - return this.warnTargetNotFound(d, p.id); + return this.warnNodeNotFound(d, p.id); } this.hoverElements((target as Node) as Element); }, @@ -435,7 +448,7 @@ export class Replayer { const event = new Event(MouseInteractions[d.type].toLowerCase()); const target = mirror.getNode(d.id); if (!target) { - return this.warnTargetNotFound(d, d.id); + return this.warnNodeNotFound(d, d.id); } switch (d.type) { case MouseInteractions.Blur: @@ -475,7 +488,7 @@ export class Replayer { } const target = mirror.getNode(d.id); if (!target) { - return this.warnTargetNotFound(d, d.id); + return this.warnNodeNotFound(d, d.id); } if ((target as Node) === this.iframe.contentDocument) { this.iframe.contentWindow!.scrollTo({ @@ -514,7 +527,7 @@ export class Replayer { } const target = mirror.getNode(d.id); if (!target) { - return this.warnTargetNotFound(d, d.id); + return this.warnNodeNotFound(d, d.id); } try { ((target as Node) as HTMLInputElement).checked = d.isChecked; @@ -590,14 +603,10 @@ export class Replayer { this.noramlSpeed = -1; } - private warnTargetNotFound(d: incrementalData, id: number) { + private warnNodeNotFound(d: incrementalData, id: number) { if (!this.config.showWarning) { return; } - console.warn( - REPLAY_CONSOLE_PREFIX, - `target with id '${id}' not found in`, - d, - ); + console.warn(REPLAY_CONSOLE_PREFIX, `Node with id '${id}' not found in`, d); } } diff --git a/src/types.ts b/src/types.ts index b5b254349f..a87275891c 100644 --- a/src/types.ts +++ b/src/types.ts @@ -136,8 +136,7 @@ export type attributeMutation = { }; export type removedNodeMutation = { - parentId?: number; - parentNode?: Node; + parentId: number; id: number; }; diff --git a/test/__snapshots__/integration.test.ts.snap b/test/__snapshots__/integration.test.ts.snap index 2a3a582ed9..04d71403f7 100644 --- a/test/__snapshots__/integration.test.ts.snap +++ b/test/__snapshots__/integration.test.ts.snap @@ -2194,10 +2194,6 @@ exports[`select2 1`] = ` { \\"parentId\\": 25, \\"id\\": 36 - }, - { - \\"parentId\\": 45, - \\"id\\": 46 } ], \\"adds\\": [