Closed oestradiol closed 2 months ago
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.
* 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 ofDefaultClient
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 likeStream
is a little misleading because of the Stream trait. Maybe we could just name itStreamClient
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?
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!
!?
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...
This Pull Request adds two new crates:
EventStreamClient
,Handlers
andSubscription
defined in atrium-streams for interacting with the variety of subscriptions in ATProto.TODO:
Firehose
implementation + add configurations to manually enable payload types to avoid unnecessary computations;com.atproto.label.subscribeLabels
;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);