Argument `idNodeMap` direct mutation

[igaloly]

In /src/snapshot.ts, snapshot function declaring a new empty map, idNodeMap, and it then returning it.

At first glance, it seems like idNodeMap is useless, because there a declare and immediate return. Only what you go inside serializeNodeWithId, see that idNodeMap is passed to and is mutated inside of it, you can understand that idNodeMap is not empty when returning it. I think it's a bit confusing and generally, as I know, not the best practice.

I suggest creating an helper function that will handle creating idNodeMap and returning it.

[Yuyz0112]

@igaloly Thanks for the feedback.

Since serializeNodeWithId will iterate the DOM tree, where would you suggest to create the idNodeMap?

[igaloly]

Maybe something like:

export function serializeNodeWithId(
  n: Node | INode,
  doc: Document,
  map: idNodeMap,
  blockClass: string | RegExp,
  skipChild = false,
  inlineStylesheet = true,
  maskAllInputs = false,
): serializedNodeWithId | null {
  const _serializedNode = serializeNode(
    n,
    doc,
    blockClass,
    inlineStylesheet,
    maskAllInputs,
  );
  if (!_serializedNode) {
    // TODO: dev only
    console.warn(n, 'not serialized');
    return null;
  }
  let id;
  // Try to reuse the previous id
  if ('__sn' in n) {
    id = n.__sn.id;
  } else {
    id = genId();
  }
  const serializedNode = Object.assign(_serializedNode, { id });
  (n as INode).__sn = serializedNode;
  map[id] = n as INode;
  let recordChild = !skipChild;
  if (serializedNode.type === NodeType.Element) {
    recordChild = recordChild && !serializedNode.needBlock;
    // this property was not needed in replay side
    delete serializedNode.needBlock;
  }
  if (
    (serializedNode.type === NodeType.Document ||
      serializedNode.type === NodeType.Element) &&
    recordChild
  ) {
    for (const childN of Array.from(n.childNodes)) {
      const serializedChildNode = serializeNodeWithId(
        childN,
        doc,
        map,
        blockClass,
        skipChild,
        inlineStylesheet,
        maskAllInputs,
      );
      if (serializedChildNode) {
        serializedNode.childNodes.push(serializedChildNode);
      }
    }
  }
  return { serializedNode, idNodeMap: map };
}

• Return an object

function snapshot(
  n: Document,
  blockClass: string | RegExp = 'rr-block',
  inlineStylesheet = true,
  maskAllInputs = false,
): [serializedNodeWithId | null, idNodeMap] {
  const { serializedNode, idNodeMap } = serializeNodeWithId( n, n, {}, blockClass, false, inlineStylesheet, maskAllInputs )
  return [
    serializedNode,
    idNodeMap,
  ];
}

• Destructure payload and return it

Even though it seems like now, serializeNodeWithId is redundant and we can make all of its logic in snapshot