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

User Streams API Support #53

Open
wants to merge 76 commits into
base: master
Choose a base branch
from
Open

User Streams API Support #53

wants to merge 76 commits into from

Conversation

kolinkrewinkel
Copy link
Collaborator

This technically works, but needs to be reviewed. Line 148 specifically. After the pause/setValue/resume exchange, the queue behaves strangely for at least one operation (not starting it.)

joeldev and others added 30 commits June 8, 2013 18:46
…lso does not remove itself as an observer: will need to be fixed.
…sses method; passes it around instead. Now we can just filter with anyObject for one with a conn. ID and no socket shuttle and go, when connecting.
@tonyarnold
Copy link
Contributor

I dunno, it just seems like good form to offer to push them back upstream and then use that as your dependency.

@berg
Copy link
Collaborator

berg commented Feb 15, 2014

Good call, @tonyarnold. My first step is going to be to take all of the substantive changes from the SocketShuttle fork, make them backwards compatible, and (try to) get them accepted upstream.

@keichan34
Copy link

This commit doesn't need to be merged, because they already fixed that issue upstream, I think.

@berg
Copy link
Collaborator

berg commented Feb 17, 2014

OK, sent in a pull request for SocketShuttle as a start: mk/SocketShuttle#3

It's basically all of the meat of Kolin's SocketShuttle patches, without the packaging changes, which I think we can accommodate inside of the ADNKit project without requiring a custom fork of SocketShuttle. This should make it easier to keep the projects manageable both by CocoaPods and git submodule users.

One thing I did run into here, which @kolinkrewinkel might have already addressed, but I haven't yet found: in my code I copy ANKClient a lot, specifically because I want to make requests with the same access token but with different pagination parameters. We should probably ensure that this doesn't mean we're creating new sockets every time (I think it's a fairly common pattern.)

@kolinkrewinkel
Copy link
Collaborator Author

Couple things: firstly, thanks. My initial inclination is towards a singleton socket, or—even better—one that sticks per authentication token.

Now that we're in the conclusion phase, I'm curious what my rationale was to destroying and recreating the socket shuttle instances on disconnect or error. In my opinion this defeats the purpose of using SocketShuttle over SocketRocket (among other things.) So, before merging, ideally, I'd like to investigate...not doing that.

Sent from my iPhone

On Feb 17, 2014, at 10:42, Bryan Berg [email protected] wrote:

OK, sent in a pull request for SocketShuttle as a start: mk/SocketShuttle#3

It's basically all of the meat of Kolin's SocketShuttle patches, without the packaging changes, which I think we can accommodate inside of the ADNKit project without requiring a custom fork of SocketShuttle. This should make it easier to keep the projects manageable both by CocoaPods and git submodule users.

One thing I did run into here, which @kolinkrewinkel might have already addressed, but I haven't yet found: in my code I copy ANKClient a lot, specifically because I want to make requests with the same access token but with different pagination parameters. We should probably ensure that this doesn't mean we're creating new sockets every time (I think it's a fairly common pattern.)


Reply to this email directly or view it on GitHub.

@tonyarnold
Copy link
Contributor

Yeah, one per token would probably be best — multi-account support would be a bit hard with a singleton. Maybe a singleton socket handler that vends socket instances per client? Would you ever need to establish more than one socket per app per client?

@kolinkrewinkel
Copy link
Collaborator Author

Not that I can think of. Even changing pagination settings wouldn't warrant creating a new socket as it's only for the short-lifetime of the request.

@berg
Copy link
Collaborator

berg commented Feb 18, 2014

I was just going to suggest creating a singleton containing a weak NSHashMap of access_token -> SocketShuttle instances and having the ANKClient lazily create (and retain) them when needed, nilling the reference when the accessToken property changes.

@tonyarnold
Copy link
Contributor

You're just looking for an excuse to use NSHashTable, @berg :trollface:

@tonyarnold
Copy link
Contributor

Has this been brought up to date as per @berg's earlier comments? I'm debating dropping this in in place of my own, manual AFNetworking/Socket-based code in a project.

Update: it might be up-to-date, but it doesn't compile out of the box on OS X. SocketRocket/SocketShuttle keep trying to build the iOS static libs 😢

ADNKit-OSX had the iOS SocketShuttle as a target dependency, fixed.
@kolinkrewinkel
Copy link
Collaborator Author

So, remaining, (and feel free to suggest, so I can add to this):

  • Test and hopefully amend destruction and recreation of socketShuttle property.
  • Change to global per-token pattern of sockets, rather than per-client.
  • Have SocketShuttle PR merged.
  • Test fresh installs with both submodules and CocoaPods.
  • Documentation
  • Merge! ✨

@tonyarnold
Copy link
Contributor

Silly question, but how do I subscribe to a user's stream? Maybe add:

  • Documentation

Update: Sorted this out privately with @kolinkrewinkel — I was on the right track, but misunderstood ADN's recommendations when starting a stream

@tonyarnold
Copy link
Contributor

In addition to the global per-token sockets, it'd be good to have a status indicator for whether the current client has an open/closed/errored socket — or some method of stopping double streaming subscriptions.

Add support for the inactive field on Channel objects
@garrettmurray
Copy link

Any update on this? Is it stable enough to use? I'm working with ADNKit now and I'd love to use user streams from the get-go.

@kolinkrewinkel
Copy link
Collaborator Author

You should be able to use this branch and stream right from the start. We just have the revisions in the checklist to do for (what we guess are) potential bugs and semantics.

@tonyarnold
Copy link
Contributor

Don't hate me, but I ran into some problems I couldn't be bothered diagnosing with AFN 1.x and started working on AFN 2.x support: https://github.com/tonyarnold/ADNKit/tree/afnetworking-2

It's mostly functional, and I think it'll end up being a bit simpler to implement/look after. It does not support anything less than iOS 7.x or OS X 10.9 though, so probably not anyone else's cup of tea at this stage 🍵

@tonyarnold
Copy link
Contributor

Something we need to add to the streaming request: configurable parameters. I just lost a morning trying to work out why I wasn't getting any annotations through with my streamed messages.

@keichan34
Copy link

@tonyarnold I'm using -(ANKGeneralParameters *) generalParameters on the main ANKClient object, is that not working for you?

@tonyarnold
Copy link
Contributor

Not in the streamed messages, no. No parameters are currently passed to the initial request for the stream, so annotations, etc aren't included. I've just pushed an update to my (getting out of control) AFNetworking 2 branch that includes this functionality.

@keichan34
Copy link

Hm, that's strange. I set generalParameters to the appropriate values (in my case, annotations and no HTML), log in with the access token (or request one if necessary), then initialize streaming. Works fine for me... (FWIW, I'm using 9aae7dd at the moment)

@tonyarnold
Copy link
Contributor

You see annotations in your streamed responses? I don't see how — they're not included by default, and the current user-streams branch doesn't ask for them… can you verify that you're seeing any annotations in the actual streamed responses (not the initial response you get when registering from a stream)?

@keichan34
Copy link

Updating to the latest commit on user-streams seemed to make the annotations go away (and the HTML to come back). I can't see anything in the commit logs that indicate things have changed, so maybe it's an updated dependency?

@tonyarnold
Copy link
Contributor

Possibly, although local testing showed that if I didn't make that initial request to https://stream-channel.app.net/stream/user with the include_annotations=1 query parameter included, no annotations come through. Maybe it's an ADN bug/change?

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.

8 participants