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

Adding net crate #123

Merged
merged 27 commits into from
Sep 17, 2024
Merged

Adding net crate #123

merged 27 commits into from
Sep 17, 2024

Conversation

kokoISnoTarget
Copy link
Contributor

@kokoISnoTarget kokoISnoTarget commented Aug 20, 2024

This adds a net crate with sync and async networking Providers as mentioned in #119 and the discord.
Please comment any thoughts regarding the implementation.

@nicoburns
Copy link
Collaborator

Oh wow. This PR makes Blitz feel fast (or at least, it did before the workaround was added). Would love to get this merged soon.

The stylesheet bug is pretty bad though. We'll need to get to the bottom of that. It looks like it might only be inline nodes that styles aren't applying to, but I'm not quite sure why that would be as it looks like that's getting rebuilt every frame.

Copy link
Collaborator

@nicoburns nicoburns left a comment

Choose a reason for hiding this comment

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

Looks like you've found the invalidation bug - nice!

I've put some review comments which I thought I had already sent, but looks like I didn't press the button. Please feel free to push back if you disagree with any of my comments: I think this is somewhere where there is quite a large design space and probably many good options.

One thing I am quite keen on is to minimise dependencies between crates. As such I would like the net crate to be as close to "url in, bytes out" as possible.

packages/dom/src/document.rs Show resolved Hide resolved
packages/dom/src/htmlsink.rs Outdated Show resolved Hide resolved
packages/net/src/provider/non_blocking.rs Outdated Show resolved Hide resolved
futures: Mutex::new(FuturesUnordered::new()),
}
}
pub fn resolve<P: From<(WindowId, T)> + Send>(
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you have fetch accept the WindowId and the callback for the result directly, then I think this resolve function can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is that basically all fetching is done before the window (and the WindowId) exist.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this could be solved by giving the Document itself an ID The ID (perhaps just a simple sequential one?) could be generated either by the high-level glue code (or by Document::new()) and then the high-level code could keep track of which windows/documents that ID corresponds to.

This seems like a good idea anyway as in future we'll likely want to support things like replacing the Document associated with a Window (e.g. when clicking on a link), and having sub-Documents within a Document (e.g. for iframes, or using DioxusDocument wrapping an HtmlDocument to implement a browser ui).

packages/net/src/lib.rs Outdated Show resolved Hide resolved
packages/net/src/provider/blocking.rs Outdated Show resolved Hide resolved
packages/net/src/lib.rs Outdated Show resolved Hide resolved
@@ -26,6 +26,9 @@ style = { workspace = true }
tracing = { workspace = true, optional = true }
blitz = { path = "../blitz" }
blitz-dom = { path = "../dom" }
blitz-net = { path = "../net" }
blitz-traits = { path = "../traits" }
html-escape = "0.2.13"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is html-escape actually used in this crate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, forgot to remove it.

git
Arc::new(callback) as SharedCallback<Resource>,
));

timer.time("Setup document prerequsits");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Typo: "prerequsits" should be "prerequisites"

Copy link
Collaborator

@nicoburns nicoburns left a comment

Choose a reason for hiding this comment

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

I still have quibbles about the structure of things here, but nothing major and I would now be keen to land this soon. The main blocker now being resolving the conflicts with the main branch.

@nicoburns
Copy link
Collaborator

Regarding the @font-face support. It would also be cool to port the woff support from #107 as a lot of webfonts seem to be in woff format. But I wouldn't block this PR on that: that could be done in a followup.

@nicoburns nicoburns merged commit 023b2f6 into DioxusLabs:main Sep 17, 2024
8 checks passed
This pull request was closed.
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.

3 participants