-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
move browser-only rrdom features to the new vdom package #913
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
Conversation
2bd6d9e to
7825edd
Compare
YunFeng0817
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
Juice10
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great stuff @Yuyz0112!
YunFeng0817
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great work!
|
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