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

Specialize HistoryProvider::on_update for web #3057

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

matthunz
Copy link
Contributor

@matthunz matthunz commented Oct 14, 2024

Removes the need for Send + Sync bounds by specializing the HistoryProvider::on_update for !Send !Sync.

Fixes #1657 by triggering switch_route on the popstate event.

/// Callback for history updates.
pub enum HistoryCallback {
    /// Local callback.
    ///
    /// This callback is only used for `dioxus-web`.
    Local(Rc<dyn Fn()>),
    /// Shared callback.
    ///
    /// This callback is used for all other Dioxus platforms to enable multi-threaded callbacks.
    Shared(Arc<dyn Fn() + Send + Sync>),
}

@ealmloff
Copy link
Member

ealmloff commented Oct 15, 2024

#[cfg(feature = "web")]
fn updater(&mut self, callback: Rc<dyn Fn()>) { ... }

#[cfg(not(feature = "web"))]
fn updater(&mut self, callback: Arc<dyn Fn() + Send + Sync>) { ... }

Specializing based on features is generally problematic because it makes the features nonn-additive. If a library activates the "web" feature on the router and I add that library as a dependency, then my code could break if I do something like this:

let updater = updater();
std::thread::spawn(|| updater);

We already have this issue in a couple different places in the dioxus crates, but we have been slowly working to remove them. For example, the default history for the router is non-additive, but will be removed in #400.

Would it be possible to implement the fix without changing the signature of updater based on feature flags? Potentially by removing the send + sync bound on other platforms or asserting that the web platform is single threaded via thread ids

@jkelleyrtp jkelleyrtp marked this pull request as draft October 31, 2024 21:14
@jkelleyrtp
Copy link
Member

I think now that the router has moved to be provided by each platform, we can simply implement this in the web crate with 0 configs.

@matthunz matthunz marked this pull request as ready for review November 3, 2024 20:51
@matthunz
Copy link
Contributor Author

matthunz commented Nov 3, 2024

Thanks! After chatting on Discord I totally get why we don't want mututally-exlcusive features.

I have a new version here using an enum to keep the public API the same. There is a hidden #[cfg(target_arch = "wasm32"] though that I hope makes sense in this context 🤔

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.

Dioxus Router’s on_update callback is not called when using browser’s back/forward buttons
3 participants