move browser-only rrdom features to the new vdom package#913
Conversation
2bd6d9e to
7825edd
Compare
YunFeng0817
left a comment
There was a problem hiding this comment.
This is a great refactor! My only concern is the name 'vdom', cause there is already a package in the npmjs's repositories. https://www.npmjs.com/package/vdom
I'm not sure whether our package still can be published or not.
f78cf0d to
18dcefb
Compare
|
@Mark-Fenng Yes, this is just a proposal, and I can refactor it. I think there are two strategies:
|
|
The This PR looks great, thanks for taking the time to break this apart! This makes building a lot cleaner, and reduces circular dependencies. I understand the confusion with BaseRRNode, I'm not sure I totally understand when to use it myself. But it makes sense to keep the imports what they where and not change them |
|
I also prefer the first strategy because it feels more intuitive. |
94738bb to
8b93765
Compare
|
@Mark-Fenng @Juice10 I've updated this PR. |
|
let's merge |
Previously, there were two parts of code in the
rrdompackage:Mixing these codes in one package cause some problems, the most obvious one is we need to import from
rrdom/es/virtual-domto make sure we will not import NodeJS-related code.Although it works, it left some tricky config code and is not friendly to bundle tools and monorepo tools.
So in this PR, I move the general implementation to a new package
rrdom, and rename the originalrrdomtorrdom-nodejs. The dependency relations become:flowchart TB rrdom --> rrweb rrdom --> rrdom-nodejs