perf(snapshot): avoid recreate element a every time

[H4ad]

Avoid recreating the same element every time, instead, we cache and we just update the element.

Before: 779k ops/s
After: 860k ops/s

Benchmark: https://jsbench.me/ktlqztuf95/1

On Chrome:

On Firefox:

[changeset-bot]

🦋 Changeset detected

Latest commit: d21a234

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 8 packages

Name	Type
rrweb-snapshot	Patch
rrweb	Patch
rrdom	Patch
rrdom-nodejs	Patch
rrweb-player	Patch
@rrweb/types	Patch
@rrweb/web-extension	Patch
rrvideo	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[eoghanmurray]

Could you also test the following possibility: [edit: this was much slower]

function getHref(doc: Document) {
  // return a href without hash
  const url = new URL(doc.location.href);
  url.hash = ''
  return url.href;
}

Some more work would need to be done to support the new customHref case so maybe this PR is fine

[eoghanmurray]

I've a version which should perform just as good without the need for caching in #1434

@H4ad would you like to run benchmarks on that?

[H4ad]

@eoghanmurray Sorry for taking so long to answer, I will run the benchmarks as soon I can and I will bring back the results

[eoghanmurray]

(in case you miss the thread further up)

Could you cherry-pick eoghanmurray@615e164

And also rebase or merge master

[H4ad]

@eoghanmurray Done!

[eoghanmurray]

I've added another review comment as I'd like to be able to recreate the SPA scenario via a test which necessitates the cache bust. I created a test case but was not able to recreate the scenario where the cache bust is needed.

[H4ad]

@eoghanmurray did you submit the review?

[eoghanmurray]

I've added another review comment

This is what I was referring to:
https://github.com/rrweb-io/rrweb/pull/1387/files#diff-6434d77e679a3d1e4217dcad03ee6f02551e41dc0b077a06a45020e68e1b4ee3R222

Here is the test I was talking about, which could be cherry-picked in here, but basically it doesn't demonstrate any need for the ./clear-current stuff, so maybe you can enhance the test case?
eoghanmurray@4d52165

And here's a refactoring of how to 'clear the cache' so that it only does it if there is an old A element, and if it's value is different to the incoming value:
eoghanmurray@38e677a

[eoghanmurray]

I propose merging a version of my changes:
38e677a

but without the This is needed for SPAs. bit as we don't yet have any evidence that that is necessary.

[H4ad]

You are right, I don't know if it was a bug on Chrome when I tested but the behavior that I described in the comment is not true anymore.

I have updated the code to reflect your changes and I also removed those comments/code about SPA.

Thanks for the patience, I was very lazy on this PR.

[eoghanmurray]

I'm assuming this is 'more correct' if the doc parameter is passed down via serializeTextNode; but I haven't checked the consequences of that.

[eoghanmurray]

To be clear, I think we need to use options.doc (and pass in doc in the options).

[eoghanmurray]

I've passed in via options.doc in #1434

[eoghanmurray]

Given that #1434 is not a goer, could you cherry pick the following commit which improves the issue raised in this thread:
eoghanmurray@615e164

[eoghanmurray]

Would you be able to cherry pick eoghanmurray@4d52165 as I think it still has worth in the event of this implementation being changed in future.

(Although 'Maintainers are allowed to edit this pull request' is checked, I'm unable to push to your H4ad origin due to other permissions issues)

[eoghanmurray]

Actually I'll just create a new PR for that test case

[eoghanmurray]

@H4ad I found a bug where a blob: style url would be persisted on the cached anchor element and would incorrect return values on subsequent calls to getHref. Fix in #1467

But could you re-review this PR in view of those single page app bugs you previously encountered?

[H4ad]

Sure, I will try to leave a review as soon as possible.

[eoghanmurray]

Just to note, using URL.canParse can make #1434 more competitve; only ~20% slower in jsbench:

function absoluteToDocCanParse(doc, attributeValue) {
// todo: do we need check if attributeValue is ''? 
    if (URL && URL.canParse(attributeValue)) {
	return new URL(attributeValue, doc.location.href).href;
    } else {
	const a = doc.createElement('a');
	a.setAttribute('href', attributeValue);
	return a.href;
    }
}