Friend list implementation #8

Closed
wants to merge 18 commits into
from

Projects

None yet

2 participants

@draivin

Implemented friend list and changed the connection to more closely resemble SteamKit's one, now the connect and logon event are separated, and you can treat for exceptions individually, also removed 'relationship' event and updated README and example.js, as the method to connect to steam changed, this is a breaking change.

@seishun
Owner

Thanks, but I'm closing this for multiple reasons.

  • There is no point in separating connection and logon. There is literally nothing a consumer would want to do after connecting other than to log in. Error handling is irrelevant, since any imaginable exceptions would occur outside of the consumer's call stack. Passing the encryption result is pointless since it will always be successful unless the protocol changes and we don't account for that by design.

  • Adding custom properties to Buffer instances is ugly in my and many others' opinion and it makes V8 unhappy because it's forced to create additional classes for each of these extra properties. I do agree that non-proto logon response should be handled gracefully, but it will require a cleaner approach.

  • Reconnection logic is botched. Right now the design is simple: it either tries to reconnect you forever, or emits an 'error' event. It reflects somewhat the behavior of an actual Steam client and it's not going to change.

  • Restructuring userinfo objects is fine, but not when it completely obliterates other important properties such as gameName (see proto file).

  • The effort to implement friend lists is laudable, but the implementation is wonky. For example, you call removeFriend on each of the already removed friends, which does nothing but confuse Steam. More importantly, without the 'relationship' event one is no longer able to listen for friend additions/removals without traversing the whole friendList object.

  • Please don't remove whitespace from empty lines. I like my whitespace.

Also, please disregard the myriad of unrelated commits above. It's the result of my evil manipulations with the commit history. Apparently GitHub doesn't handle rebases too well.

@seishun seishun closed this Mar 1, 2013
@draivin

There is no point in separating connection and logon.

I do believe even if you don't present this to the user, you should separate the methods internally, but this is just personal opinion.

Reconnection logic is botched. Right now the design is simple: it either tries to reconnect you forever, or emits an 'error' event. It reflects somewhat the behavior of an actual Steam client and it's not going to change.

At least on my tries, when the connection times out there is no _sessionID on the bot object, so the retry code never gets called.

Restructuring userinfo objects is fine, but not when it completely obliterates other important properties such as gameName

This is a simple change on the personaState handler.

The effort to implement friend lists is laudable, but the implementation is wonky. For example, you call removeFriend on each of the already removed friends

That was a misread on my part of SteamKit's code, they remove it from the local friend list, I read it as if they asked the server for removal, my bad.

Please don't remove whitespace from empty lines. I like my whitespace.

That was my editor's doing, my bad.

Overall, I just went in a direction to try and make it closer to the SteamKit's implementation.

Oh, and I have one question, if you know the answer it would be great:
When I add Steam.EClientPersonaStateFlag.Presence to the ClientRequestFriendData request I don't see any changes in the response, is this behaving correctly? And if so, what would be its use?

PS: I seem to cross with your pull requests everywhere steam and node meet, last time was when I went looking for a steam openID implementation and found your pull request there.

@seishun
Owner

At least on my tries, when the connection times out there is no _sessionID on the bot object, so the retry code never gets called.

Yeah, current master doesn't reconnect if the initial connect fails. But it should emit an 'error' event. Otherwise it's a bug. (cf. 'testing' branch)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment