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

Latest updates and changes #37

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

Latest updates and changes #37

wants to merge 39 commits into from

Conversation

thewoodfish
Copy link
Member

Breaking Changes in the Latest Version

In the latest version of our library, we have introduced two major updates that bring significant improvements but also include breaking changes:

  1. Better Handling Interface for Network Events:

    • Old Method: Previously, users had to implement a network events trait to define event handler functions.
    • New Method: Now, events are simply pushed into an internal buffer that can be polled and consumed to get events.
    • Advantages: This approach prevents potential deadlocks and makes it easier to delay the firing of handlers. It also allows different handlers to manage the same events at the point of consumption.
    • Downside: Events can be lost if the queue becomes full and overflows.
  2. Separation of Application Data from the Network Layer:

    • Old Method: The application layer used to pass state to the network, which would keep and expose it back.
    • New Method: The network layer is now pure and independent of any application data and state. It strictly handles network operations without storing or managing any data state.
    • Advantages: This separation ensures a clean and independent network layer that is focused solely on handling network events.

These updates are significant and introduce breaking changes. Please refer to the updated documentation for guidance on how to migrate to the new version.

@thewoodfish thewoodfish requested a review from sacha-l June 22, 2024 15:07
Copy link
Collaborator

@sacha-l sacha-l left a comment

Choose a reason for hiding this comment

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

Really awesome changes, adding more elegance and robustness to the foundations. Decoupling the AppState from Core makes things a lot more breathable.

Here's a first pass of revision. Everything in this review is really just documentation cosmetics. I made a commit to update some of the docs on the variants (was easier than to do in the Github UI). Before merging this PR and because it contains major breaking changes, I will do another pass on this for:

  1. Testing all core library code again
  2. Running the examples

Wanted to get this first review in quick. Cheers!

tutorials/file_sharing_app/src/main.rs Outdated Show resolved Hide resolved
tutorials/file_sharing_app/src/main.rs Outdated Show resolved Hide resolved
tutorials/file_sharing_app/src/main.rs Outdated Show resolved Hide resolved
tutorials/file_sharing_app/src/main.rs Outdated Show resolved Hide resolved
tutorials/simple_game/src/main.rs Outdated Show resolved Hide resolved
swarm_nl/src/core/prelude.rs Outdated Show resolved Hide resolved
swarm_nl/src/core/prelude.rs Outdated Show resolved Hide resolved
swarm_nl/src/core/prelude.rs Outdated Show resolved Hide resolved
swarm_nl/src/core/prelude.rs Outdated Show resolved Hide resolved
swarm_nl/src/core/prelude.rs Outdated Show resolved Hide resolved
@sacha-l
Copy link
Collaborator

sacha-l commented Jul 23, 2024

@thewoodfish : can you go ahead an commit my suggestions or comment on anything I may've missed please? 🙏🏻

Then I'll be able to pull locally and make some additional amendments in a separate commit as part of my second review.

@sacha-l
Copy link
Collaborator

sacha-l commented Jul 29, 2024

For code review and test coverage [WIP]. Tests to write around events are pushed into an internal buffer that gets polled.

    1. Test for Polling works
    2. Test for limits of internal buffer 
    3. Test consuming different events and using them works as expected
    4. Test for deadlock situation handled with elegance works 
    5. Test that different handles to manage the same event works

More to consider: updates / end-user docs for adapting to breaking changes -- these need updating.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants