[Proposal] Resolve node ID's on record-side

[marcospassos]

I want to discuss the feasibility involves resolving all node ids at record-time instead of replay-time, as it'd make the code a lot simpler and more consumer-friendly.

I've implemented the following POC that resolves node ids regardless of the order in which the observer reports them:

private resolveAddedNodes(mutations: addedNodeMutation[]): addedNodeMutation[] {
    const previous: { [key: number]: number } = {};
    const next: { [key: number]: number } = {};

    for (let mutation of mutations) {
        if (mutation.previousId != null && mutation.previousId >= 0) {
            previous[mutation.previousId] = mutation.node.id;
        }

        if (mutation.nextId != null && mutation.nextId >= 0) {
            next[mutation.nextId] = mutation.node.id;
        }
    }

    let additions: addedNodeMutation[] = [];

    for (let mutation of mutations) {
        let {previousId, nextId} = mutation;

        if (previousId !== null && previousId < 0) {
            previousId = next[mutation.node.id];
        }

        if (nextId !== null && nextId < 0) {
            nextId = previous[mutation.node.id];
        }

        additions.push({
            parentId: mutation.parentId,
            nextId: nextId,
            previousId: previousId,
            node: mutation.node,
        });
    }

    return additions;
}

Could you check the feasibility of this proposal?

[Yuyz0112]

@marcospassos when would you like to execute this method, after a mutation callback?

[marcospassos]

I'd suggest before emitting to listeners, in the recorder

[Yuyz0112]

@marcospassos So it will execute in this order, right?

1. mutation observer callback
2. loop mutation records
3. resolve nodes
4. emit to listeners

According to this commit, sometimes the newly added nodes will not show in the same mutation observer callback loop.

[marcospassos]

@Yuyz0112 can you provide a real example or test when it occurs? When it happens, what is missing? Parents, siblings, or both? Can we use a buffer in the record side, instead of the replay side?

[Yuyz0112]

@marcospassos I could not reproduce that with any test suite code. And since mutation observer callback is a microtask in the event loop, your code should work and be safe.

So how about adding this resolve logic in the record side, but also keep the replay side missing node resolver logic as a double check?

[marcospassos]

Thank you for analyzing it more in-depth, @Yuyz0112.

Although I see no problem in leaving this fallback check, my personal opinion is that we should remove that part of the code until it proves necessary. Otherwise, we will never be sure that it is necessary, and we will carry this legacy code forever. If it does, at some point we'll notice that and reverse the commit that removed it.

[Yuyz0112]

@marcospassos

Agree with that. The only problem is some users may not report the issue and jus stop using it:)
So I would like to remove the resolving node logic at replay side in a single commit then we can revert it if necessary. Also, add some warning messages when users meet node with -1 id and taught them to report that so we can figure out what's happening.

BTW, would like to make a PR for this proposal in the record side?

[marcospassos]

Sure :)

[marcospassos]

@Yuyz0112 I did not forget this - I just didn't have time to work on it yet.

[eoghanmurray]

I just wanted to add my 2c here w.r.t. 661fa6c

#224 added the concept of 'freezing' mutations, i.e. not emitting them immediately (if the browser page is not currently open).

I wonder could we apply a similar idea here, i.e. if we can't resolve all nodes, just let those mutation events build up until a subsequent mutation fully resolves the tree. (I don't have first hand experience of unresolved nodes etc.)