Use of `dyn FnMut` precludes recursively called callbacks

[ctm]

I'm using wasm-sockets in closed-source software I've written that opens pop-up windows. My software sometimes fails under Firefox. It's sufficiently complicated that I created a repository that describes the problem and the workaround.

I'm currently using a fork of wasm-sockets where I've simply replaced dyn FnMut with dyn Fn, but I haven't created a pull-request because I know that's making the callbacks more restrictive. On the other hand, I couldn't think of a nice clean way to give the user of wasm-sockets the choice.

So, @scratchyone do you have any thoughts? FWIW, reqwasm and the wasm-bindgen WebSockets example both have this problem too. I am writing to you first though, because I'm actually using wasm-sockets.

[scratchyone]

Thanks so much for your fork and the super detailed descriptions, those made understanding the issue super simple! I think changing to dyn Fn completely would be a pretty large loss of usability for most library users, because a lot of people might never encounter that Firefox bug and it would make mutating outside variables very painful for users of the EventClient. Plus it would require making a pretty big breaking change.

What would your thoughts be on instead using a feature flag to switch to dyn Fn, plus an explanation in the Readme about the bug, with a link to your writeup and a guide on how to enable the feature flag to fix it? To me, that seems like the best way to fix the bug without making a large breaking change.

[scratchyone]

Update, I just looked over the code and I realized I was mistaken. All of the set_on functions already take a dyn Fn as their input anyways, so this wouldn't actually be a breaking change! It appears that the dyn FnMut usage is exclusively internal and unneeded already, so as long as the library continues to compile and work correctly, it shouldn't cause any problem to switch them all over to dyn Fn.

If you're willing to open a PR, I'd happily merge that, or I could just manually merge your branch if you don't feel like opening one.

[ctm]

I'm crazy tied up right now. It's up to you. What do you want in the PR? Do you want a better commit message? Should I add some comments to the code itself? I'm fine with you doing anything you want, but I also don't want to push work off onto you that I should do.

On Thu, Jan 20, 2022 at 3:42 PM scratchyone @.***> wrote:

Update, I just looked over the code and I realized I was mistaken. All of the set_on functions already take a dyn Fn as their input anyways, so this wouldn't actually be a breaking change! It appears that the dyn FnMut usage is exclusively internal and unneeded already, so as long as the library continues to compile and work correctly, it shouldn't cause any problem to switch them all over to dyn Fn.

If you're willing to open a PR, I'd happily merge that, or I could just manually merge your branch if you don't feel like opening one.

— Reply to this email directly, view it on GitHub https://github.com/scratchyone/wasm-sockets/issues/1#issuecomment-1017990868, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAGNBIBC6ATAQFCICP2EITUXCFT3ANCNFSM5MHSMKVQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you authored the thread.Message ID: @.***>

-- Cliff Matthews @.***

[scratchyone]

Awesome, with that this bug should be fixed! I have also pushed version 0.2.2 to crates.io with all the new changes. Thank you so much for all the work you put into figuring out the cause of this bug and writing all the code to fix it.