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

atrium-streams + atrium-streams-client implementation #223

Closed
wants to merge 5 commits into from

Conversation

oestradiol
Copy link
Contributor

@oestradiol oestradiol commented Sep 12, 2024

This Pull Request adds two new crates:

  • atrium-streams: Definitions for traits, types and utilities for dealing with event stream subscriptions.
  • atrium-streams-client: A library that provides default implementations of the EventStreamClient, Handlers and Subscription defined in atrium-streams for interacting with the variety of subscriptions in ATProto.

TODO:

  • - Implement processing of other payload types for Firehose implementation + add configurations to manually enable payload types to avoid unnecessary computations;
  • - Implement other subscriptions, such as com.atproto.label.subscribeLabels;
  • - Using std::mem::transmute is not recommended. It works wonderfully for now, but eventually it might not anymore. Think of a way to replace the outdated rs_car crate somehow (if possible);
  • - Create crate features so that users can deactivate specific features;
  • - Write unit tests;
  • - Write CHANGELOG.md and README.md.
  • - Prepare for wasm target;

@sugyan
Copy link
Owner

sugyan commented Sep 12, 2024

Thank you for the pull request!

It is great that the crate is divided into two, one for definitions such as traits, and one for providing concrete implementations.

However, I would like you to reconsider the crate name.
My thoughts are as follows:

  • Is "subscription" included in XRPC?
    I do not understand the exact definition of XRPC, but https://atproto.com/specs/event-stream is listed in a separate section as "HTTP API (XRPC)". The endpoint is indeed "/xrpc/{nsid}"... but I felt that there is no need to include "xrpc" in the crate name.

  • Should we use "was"?
    My understanding from reading https://atproto.com/specs/event-stream is that "WebSocket is used as the initial transport for event streams" and that it may become something else in the future.
    I thought that the name WssClient would be fine for the implementation provided as the default client, but for the name of the trait provided by the library, it would be better to use a more abstract name, such as "stream".

I'd love to hear your thoughts too.

@oestradiol
Copy link
Contributor Author

* Is "subscription" included in XRPC? [...] The endpoint is indeed `"/xrpc/{nsid}"` [...]

Hmm yes, I think so? Because of the endpoint, as you said.

  I do not understand the exact definition of XRPC, but https://atproto.com/specs/event-stream is listed in a separate section as "HTTP API (XRPC)"

Honestly, me neither, it is a little confusing hahah

  but I felt that there is no need to include "xrpc" in the crate name.

Hmm okay, I agree with that. The name is a little too verbose.

* Should we use "wss"?
  My understanding from reading https://atproto.com/specs/event-stream is that "WebSocket is used as the initial transport for event streams" and that it may become something else in the future.
  I thought that the name `WssClient` would be fine for the implementation provided as the default client, but for the name of the trait provided by the library, it would be better to use a more abstract name, such as "stream".

I agree that the name WssClient would be better used for the default client, instead of DefaultClient itself. So yeah, we can change this one.
Now, for the trait provided, I am not really sure what name we should use. I feel like Stream is a little misleading because of the Stream trait. Maybe we could just name it StreamClient instead? I like that better.


All that being said, for the trait names, I propose something like atrium-streams and atrium-streams-client. What do you think?

@sugyan
Copy link
Owner

sugyan commented Sep 13, 2024

Thank you for considering this!
Yes, I feel that StreamClient or EventStreamClient would be a good name for the trait name, and atrium-streams and atrium-streams-client seem very suitable for the crate names!

@oestradiol oestradiol changed the title atrium-xrpc-wss + atrium-xrpc-wss-client implementation atrium-streams + atrium-streams-client implementation Sep 13, 2024
@oestradiol oestradiol force-pushed the main branch 2 times, most recently from 41ca249 to a42ff5b Compare September 13, 2024 03:29
@oestradiol oestradiol deleted the branch sugyan:main September 13, 2024 20:02
@oestradiol oestradiol closed this Sep 13, 2024
@oestradiol oestradiol deleted the main branch September 13, 2024 20:02
@oestradiol
Copy link
Contributor Author

oestradiol commented Sep 13, 2024

!?

Well, good to know! Apparently GitHub closes Pull Requests if you rename a branch!

That is a very unexpected behaviour that I did not know of, wow...

I apologise. I'll open another pull request, since it won't let me re-open this one...

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