sycamore-rs / sycamore

A library for creating reactive web apps in Rust and WebAssembly
https://sycamore-rs.netlify.app
MIT License
2.72k stars 145 forks source link

Probable bug in signal propagation #641

Closed Jan-V2 closed 8 months ago

Jan-V2 commented 8 months ago

Describe the bug Dear sycamore team, i believe i have run into a bug, related to signal propagation. I'm switching to a different framework, for other reasons, so do not have the energy to write a more extensive breakdown. If you think that this submission is too much trouble to address, then i understand.

To Reproduce the bug can be reproduced as follows:

  1. clone the repo at https://github.com/johnvanderholt/wasm_risk/tree/387e7a4da53cbb7516d569309ce387871b70ae7b and run the code using sh run.sh
  2. select two players in the setup menu until you get to the army placement menu
  3. placing armies until the ui switches over to the next player and then clicking one more time, will produce the following output, in the console.
updating start placement info StartArmyPlacementInfo { is_done: true, updated: true, current_player: 0, num_players: 2, armies_per_player: [0, 21, 0, 0, 0, 0, 0, 0] } // RcSignal gets set
checking eq
updating placement start from rc // RcSignal gets propagated to signal, the component is updated, but the Signal state is not propagated back to the RcSignal, therefore current_player is not updated in the RcSignal
running placement id 34, start true
updating start placement info StartArmyPlacementInfo { is_done: true, updated: true, current_player: 0, num_players: 2, armies_per_player: [0, 21, 0, 0, 0, 0, 0, 0] } // RcSignal gets set again
checking eq 
updating placement start from sig // because of accidental behavior in my code, this time the Signal does get propagated back to the RcSignal

Expected behavior Canvas handler -> Game stuct -> updates RcSignal -> cloned to Signal -> Component update -> Signal updated - > RcSignal updated The RcSignal update at the end only happens if the state is set twice.

Environment

Additional context There are two bools called is_broken inside the code, which if both set to true will run a later version of the code, which panics, even though it shouldn't, because the state doesn't get propagated all the way. Anyway, i hope this submission is at least mildly useful, and i wish you good luck with your project.

lukechu10 commented 8 months ago

I believe that this should be fixed by #612 which makes removes RcSignal in favor of making all signals Copy + 'static.

Jan-V2 commented 8 months ago

I've updated the code to use sycamore 0.9.0-beta.2 and the bug is still present. The new signals do mean, that i'll be able to refactor the code base, which will probably solve the bug in my instance. But this could be something someone else might run into in a more complex project than mine, Although i admit that it might be a bit of an extreme case.

Here's the link to the new version https://github.com/johnvanderholt/wasm_risk/tree/8afca8598d683801882f542ed53cae5a5e1441c4

lukechu10 commented 8 months ago

I've updated the code to use sycamore 0.9.0-beta.2 and the bug is still present. The new signals do mean, that i'll be able to refactor the code base, which will probably solve the bug in my instance. But this could be something someone else might run into in a more complex project than mine, Although i admit that it might be a bit of an extreme case.

Here's the link to the new version https://github.com/johnvanderholt/wasm_risk/tree/8afca8598d683801882f542ed53cae5a5e1441c4

I've had a quick look at your code and I believe the problem might be due to updating signals from within memos/effects.

For one thing, if you are not using the result of the memo, it is semantically more correct to use create_effect which is specifically meant for this kind of thing.

Secondly, it is generally considered much better to use memos instead of manually updating signals withy effects. This not only solves some synchronisation issues and the "diamond problem", it also generally should have much better performance.

Hope this helps!

Jan-V2 commented 8 months ago

ok, good to know. thanks, for the help.