Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rrdom #613

Merged
merged 16 commits into from
Jan 11, 2022
Merged

rrdom #613

merged 16 commits into from
Jan 11, 2022

Conversation

YunFeng0817
Copy link
Member

@YunFeng0817 YunFeng0817 commented Jul 5, 2021

Purposes

  1. Do efficient analysis without the dependency of the browser environment.
  2. Make use of rrdom to implement a more accurate algorithm that can calculate the DOM diff without real DOM operations, like what virtual DOMs do.

Milestones

  • Be able to create a rrdom tree according to a full snapshot and a real dom. Be able to manipulate tree nodes like Dom APIs. Be able to run rrweb replayer on rrdom runtime.
  • Implement rrdom diff algorithm and replace the virtual parent optimization with it.
  • Implement some query APIs like querySelector, create time-sensitive indexes and provide some useful analysis API for users.
  • Implement rrdom in other programming languages like Rust.

@YunFeng0817 YunFeng0817 marked this pull request as draft July 5, 2021 04:16
@YunFeng0817
Copy link
Member Author

@Yuyz0112 @eoghanmurray @Juice10 PTAL and make sure we are heading in the right direction.

@Juice10
Copy link
Contributor

Juice10 commented Jul 5, 2021

@Mark-Fenng impressive work so far!
Personally I would try not to do the css parsing myself, its a lot of work and there are many edge cases to consider. I'd go for something like cssstyle it's used by jsdom which is quite widely used and already has a lot of edge cases worked out for you.

@YunFeng0817
Copy link
Member Author

@Juice10 Agreed! I have tried cssstyle and it works well in the nodejs environment but I found some problems in the browser environment. So I have to remove it temporarily in the commit d2fadec.
Maybe If we can handle that compatible problems, cssstyle is the best choice for us?

@Juice10
Copy link
Contributor

Juice10 commented Jul 5, 2021

@Mark-Fenng cool, happy to see you already went down that road!
Since rrdom is meant to be used both server side and client side as a virtual dom. I wonder if it makes sense only use cssom in the node environment, and let let the browser do the css parsing on the client side.
We might need to use document.createElement().style to get access to that on the client side, but as long as we don't attach those nodes to the Dom it should be a lot faster and a lot less error prone than using regular expressions (especially on large or complicated styles).

@Yuyz0112
Copy link
Member

Yuyz0112 commented Jul 5, 2021

@Mark-Fenng Does the one rrweb-snapshot use helps?

@YunFeng0817
Copy link
Member Author

@Yuyz0112 css doesn't support the mutual conversion between inline-style and cssText.

@eoghanmurray
Copy link
Contributor

@Mark-Fenng can you tell me about the virtual parent optimization, where in the current codebase is it?

@YunFeng0817
Copy link
Member Author

@eoghanmurray
The main code is here.
The mechanism is that during the seeking process, we remove some root elements(parent elements) from the DOM, and then mutation of their children will not cause reflow, which is relatively slow. After the seeking is finished, we put these root elements back to the DOM. In this way, number of reflows is significantly reduced so that we're able to accelerate the performance of the seeking process.

YunFeng0817 and others added 13 commits December 7, 2021 19:06
Errors are caused by the declaration similarity of @types/mocha and @types/jest if we install both of them in the whole project.
This mirrors the `Element.tagName` implementation:

```
For DOM trees which represent HTML documents, the returned tag name is always in the canonical upper-case form. For example, tagName called on a <div> element returns "DIV".
```
https://developer.mozilla.org/en-US/docs/Web/API/Element/tagName
Since Event class is built in nodejs after v15.0.0
@YunFeng0817 YunFeng0817 marked this pull request as ready for review December 7, 2021 11:15
@YunFeng0817
Copy link
Member Author

I think I have mostly completed the content of stage 1.
Now, we can run rrweb.Replayer on nodejs in this way:

import { Replayer } from 'rrweb';
const fs = require('fs');
require('rrdom');

var events = JSON.parse(
  fs.readFileSync(require.resolve('./events.json')).toString(),
); // Load rrweb events from a file.
var replayer = new Replayer(events, {
  showWarning: false,
});
replayer.play();
replayer.on('finish', () => {
  console.log("playback finished!");
});

I hope the current work could be merged into the master branch without being squashed.

@Juice10
Copy link
Contributor

Juice10 commented Dec 14, 2021

Absolutely loving this @Mark-Fenng! I'm reading through your PR, review will come soon!

I noticed that packages/rrdom/src/document-nodejs.ts and packages/rrdom/src/document-browser.ts are 90% the same.
Would it be possible to have one inherit from the other so changes don't have to be made twice?

Copy link
Contributor

@Juice10 Juice10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This honestly looks great! If we can get some of the duplication between the document-browser.ts and the document-nodejs.ts down I think this is ready to merge.

packages/rrdom/src/document-browser.ts Outdated Show resolved Hide resolved
packages/rrdom/src/document-browser.ts Outdated Show resolved Hide resolved
packages/rrdom/src/document-browser.ts Outdated Show resolved Hide resolved
@YunFeng0817
Copy link
Member Author

I noticed that packages/rrdom/src/document-nodejs.ts and packages/rrdom/src/document-browser.ts are 90% the same. Would it be possible to have one inherit from the other so changes don't have to be made twice?

@Juice10 Your concern is absolutely reasonable. I split two versions of rrdom because rrdom for nodejs is getting bigger and it's not suitable for running on browsers. document-browser.ts is built for virtual dom optimization in rrweb replayer and it's in an initial state so that it's maybe not a proper time to do de-duplication work now. How about avoiding merging it into the main branch until it's finished and in a good code style?

@YunFeng0817 YunFeng0817 changed the title rrdom drafts rrdom Dec 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants