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

fix #83, and change events like https://github.dev/centrifugal/centri… #84

Merged
merged 7 commits into from
Jan 20, 2024

Conversation

hetao29
Copy link
Contributor

@hetao29 hetao29 commented Jan 15, 2024

@FZambia
Copy link
Member

FZambia commented Jan 16, 2024

Hello, @hetao29 - please revert 16a13c3 - I'll investigate CI issue, but the code you removed is generally useful - it's required for package to work in web environment.

…teBuffer' isn't defined for the type 'WebSocket'"

This reverts commit 16a13c3.
@FZambia
Copy link
Member

FZambia commented Jan 17, 2024

@hetao29 hello, I fixed CI issue in terms of #85 - could you please rebase to master? Or simply open new PR - whatever you prefer.

@hetao29
Copy link
Contributor Author

hetao29 commented Jan 17, 2024

@hetao29 hello, I fixed CI issue in terms of #85 - could you please rebase to master? Or simply open new PR - whatever you prefer.

merged.


final String user;
final String client;
final proto.ClientInfo info;
Copy link
Member

Choose a reason for hiding this comment

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

We need to keep backwards compatibility of event fields - so instead of removing user and client we need to extend events with connInfo and chanInfo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Personally, I think it is really important to maintain compatibility, but the interface unity of the external output SDK is also very critical. In this SDK, client and user are different from other languages. It feels like it is more superfluous, so after referring to other After installing the sdk, I decided to delete it directly instead of simply adding an info

Copy link
Member

Choose a reason for hiding this comment

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

I mean events should be like this:

class JoinEvent {
  ...

  final String client;
  final String user;
  final List<int>? connInfo;
  final List<int>? chanInfo;
  
  ...
}  

This will allow us releasing new version of centrifuge-dart and not break other users' code upon upgrade. We can break compatibility if there is enough reasoning for it - but in this case it seems we can simply extend current events with new optional fields from ClientInfo. Or having final ClientInfo info; simplifies sth for your use case to justify breaking change at this point?

BTW: you are exposing proto.ClientInfo now, but we have another ClientInfo in events.dart to not export Protobuf structures to library users directly. See ClientInfo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, cool, I'll repush later...

@FZambia FZambia self-requested a review January 20, 2024 13:05
Copy link
Member

@FZambia FZambia left a comment

Choose a reason for hiding this comment

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

Many thanks 👍

@FZambia FZambia merged commit bd54ce6 into centrifugal:master Jan 20, 2024
3 checks passed
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