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

Refactor project #204

Closed
wants to merge 4 commits into from
Closed

Conversation

yorickvanzweeden
Copy link

@yorickvanzweeden yorickvanzweeden commented Sep 20, 2024

Hi @silentworks

This is a bit of an opinionated PR, so I understand if you are not willing to merge it. But I think it is quite an improvement w.r.t. to the current package.

Let me know!

What is the current behavior?

The current library has some issues and was not working for me in a couple of ways:

  • The package does not setup logging in all modules properly (i.e. compare client.py and push.py)
  • The auto_reconnect did not work, and after a few hours the connection would drop without error
  • There are multiple instances of default mutable arguments
  • There are broken method calls (PR203)
  • The docs folder is outdated by 2 years

Personally, I found the mix of async / sync not that intuitive. Other Supabase libraries seem to stick to async for callbacks too.

What is the new behavior?

I have tried to maintain the same interface, to induce as little breaking changes as possible.

I have a list of larger changes:

  • Use async / await throughout the package
    • Note: This does require async callbacks from the user, as opposed to the sync
  • Logging is more abundant and properly initiated
  • Properly handle websocket disruptions, including rejoining channels
  • Heartbeat is initiated on connection, rather on listen. This aligns with realtime-js
    • Note: A separate call to client.listen is no longer needed

Smaller additions:

  • More exceptions types to better distinguish exception events
  • Use dataclass annotations for simple classes
  • Stronger typing for presence
    • Note: This changes the callback signature for presence joining and leaving
  • More robust URL rewriting

- Introduce `RealtimeError` as the base class for exceptions; update custom exceptions to inherit from it.
- Add new exceptions: `ConnectionFailedError`, `ReconnectionFailedError`, `InvalidMessageError`.
- Refactor asynchronous classes (`AsyncRealtimeClient`, `AsyncRealtimeChannel`, `AsyncPush`, `AsyncTimer`, `AsyncRealtimePresence`) for proper async handling, better logging, and error handling.
- Improve URL handling using `urllib.parse` for accurate WebSocket URL construction.
- Use `@dataclass` for data structures where appropriate.
@silentworks
Copy link
Contributor

I applaud your effort in this PR but making such huge changes on so many files is not easy to review. It's better if you create smaller PRs that are easier to review like you did with #203. This way we can review and accept PRs into the project easier. I haven't merged #203 as yet because I'm setting up a test project to test out the changes, once that's done it will make reviewing and merging these PRs faster.

@yorickvanzweeden
Copy link
Author

Understandable! My primary purpose was to get the library working for my internal projects. I am closing the PR for now, as I can't be sure I have the time to create many smaller structured PRs

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.

2 participants