polyphony-chat / chorus

A rust library for interacting with multiple Spacebar-compatible Instances at once.
https://crates.io/crates/chorus
Mozilla Public License 2.0
16 stars 7 forks source link

Re-evaluate `ChorusUser::shell` #494

Closed bitfl0wer closed 2 months ago

bitfl0wer commented 2 months ago

https://github.com/polyphony-chat/chorus/blob/ca9c4b8949c021822b7ff974c15d5d3575b53c15/src/instance.rs#L208-L232

Re-evaluate if ChorusUser::shell is still needed, and if so, if it is implemented correctly. I remember that this method was used as a sort of "hack" a couple months ago, but the code has changed a lot since then, so it might not be needed anymore.

kozabrada123 commented 2 months ago

It is actually still used in several places:

https://github.com/polyphony-chat/chorus/blob/ca9c4b8949c021822b7ff974c15d5d3575b53c15/src/api/auth/login.rs#L17-L34

https://github.com/polyphony-chat/chorus/blob/ca9c4b8949c021822b7ff974c15d5d3575b53c15/src/api/auth/register.rs#L21-L41

https://github.com/polyphony-chat/chorus/blob/ca9c4b8949c021822b7ff974c15d5d3575b53c15/src/api/users/users.rs#L152-L161

(This one should likely at least support a version with an existing user):

https://github.com/polyphony-chat/chorus/blob/ca9c4b8949c021822b7ff974c15d5d3575b53c15/src/api/users/users.rs#L117-L130

kozabrada123 commented 2 months ago

Also, note that despite ChorusUser including a gateway handle

https://github.com/polyphony-chat/chorus/blob/ca9c4b8949c021822b7ff974c15d5d3575b53c15/src/instance.rs#L208-L218

login and register spawn a new gateway, which should probably be fixed :P

https://github.com/polyphony-chat/chorus/blob/ca9c4b8949c021822b7ff974c15d5d3575b53c15/src/api/auth/login.rs#L21-L45

https://github.com/polyphony-chat/chorus/blob/ca9c4b8949c021822b7ff974c15d5d3575b53c15/src/api/auth/register.rs#L25-L54

bitfl0wer commented 2 months ago

I sort of have an issue with the ::shell function even needing to exist in the first place, seems like an architectural blunder from my side. But maybe it's also fine and I am reading too much into this 🥲

kozabrada123 commented 2 months ago

If possible, I think it's better not to have sth like this is the first place, though I don't have that much of an issue with using it

kozabrada123 commented 2 months ago

either way functions like register and login should probably expand the created shell, instead of using it for one request and then building a new user

kozabrada123 commented 2 months ago

Oh dear, I just found this:

login:

register:

(In practice this probably isn't as terrible as it sounds though, since only the last gateway is used for authenticating and the others are almost immediately dropped and closed)

bitfl0wer commented 2 months ago

Oh dear lord

what have I done

bitfl0wer commented 2 months ago

To be fair, it's ugly, but it works, and it's not like this has affected the rest of the software architecture or performance in any significant way, so it's "okay"

kozabrada123 commented 2 months ago

Sure, but I'd still rather fix it

bitfl0wer commented 2 months ago

Absolutely! Hence the issue

kozabrada123 commented 2 months ago

I think the way we use shell is now pretty alright, I don't think we need to change the method itself