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

Hot reloading support #67

Merged
merged 16 commits into from
Jul 2, 2024
Merged

Hot reloading support #67

merged 16 commits into from
Jul 2, 2024

Conversation

matthunz
Copy link
Contributor

@matthunz matthunz commented Jun 18, 2024

Connects blitz to the dioxus-hot-reload crate by propagating tao events to the DioxusDocument.

Blitz examples still don't seem to work correctly, I think it has something to do with the virtual manifest?

@matthunz matthunz marked this pull request as draft June 18, 2024 20:35
@matthunz matthunz marked this pull request as ready for review June 20, 2024 22:21
euclid = { version = "0.22", features = ["serde"] }
# mozbuild = "0.1.0"

[dev-dependencies]
blitz = { path = "./packages/blitz" }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could this change be causing the examples not to work correctly?

Comment on lines 15 to 16
dioxus-cli-config = { git = "https://github.com/dioxuslabs/dioxus", rev = "a3aa6ae771a2d0a4d8cb6055c41efc0193b817ef"}
dioxus-hot-reload = { git = "https://github.com/dioxuslabs/dioxus", rev = "a3aa6ae771a2d0a4d8cb6055c41efc0193b817ef"}
Copy link
Collaborator

Choose a reason for hiding this comment

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

These should be optional, based on the hot-reload feature.

Comment on lines 220 to 227
let parent_id = node.parent.unwrap();

// Get this node's parent, or the root node if it has none.
let parent_id = node.parent.unwrap_or_default();
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't seem right. Only the root node should have no parent. And we wouldn't want to insert a child of the root node instead of a sibling. What is the use case here? Replacing the root node?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@matthunz This is the only thing blocking this PR as far as I'm concerned. I'd like to understand why this is required. It seems wrong that WriteMutations::insert_nodes_after would be called on a node without a parent. @ealmloff Any thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think this should be necessary. The root node should be ElementId(0) and every node except that node should have a parent in dioxus mutations. Dioxus will never insert a node after or before ElementId(0)

Copy link
Member

@ealmloff ealmloff left a comment

Choose a reason for hiding this comment

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

The hot reloading part of this looks good! It is very similar to how desktop does things which is nice

@jkelleyrtp jkelleyrtp merged commit 3e58462 into DioxusLabs:main Jul 2, 2024
8 checks passed
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.

4 participants