Skip to content
This repository has been archived by the owner on May 20, 2021. It is now read-only.

Extending the METDDPClientDelegate protocol #17

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tonyxiao
Copy link

Adding a lot of fine grained notifications to delegate. This makes it much easier to coordinate application state based on the overall state of meteor rather than trying to cobble together the full story via individual callback handlers and notifications.

Also allowing the delegate to peek into what's going on at the connection level to do things like custom logging for debugging purposes.

I tried to make the minimum number of changes needed to accomplish this and stay within the coding style / convention of the library. Hope it hopes!

@tonyxiao
Copy link
Author

Adding a few more things. The other problem that this pull request is designed to solve is to get more fine grained control over exactly when connection happens. Right now if there is a cached account in the keychain then simply by allocating an instance of METDDPClient will already cause it to auto-connect to the server. (Due to callback from network activity monitor)

@tonyxiao
Copy link
Author

There are also other scenarios where one might need to connect to multiple DDP servers that certainly don't share the same account. Having account be a dependency that can be passed in feels like a reasonable choice.

@martijnwalraven
Copy link
Owner

Thanks! I agree being able to pass in account is a good solution, and I can also see how adding delegate notifications for global concerns like logging, errors, login and logout might be useful. I'm less sure about offering delegate notifications as alternatives to callbacks for subscriptions and method invocations however (and about exposing METMethodInvocation).

Could you give an example of how you would be using these notifications and what you mean by coordinating application state? What part of the application would act as a delegate? What would it do with the knowledge that a method returned a certain result for instance? What is wrong with using a callback for this?

I hope you don't mind me asking these questions, but I don't want to overcomplicate the public interface unnecessarily.

@tonyxiao
Copy link
Author

Let me add my thoughts, also I think I'm gonna clean the commits up a little and separate them into multiple pull requests because there's been a lot of different ideas that went into the master commits.

@tonyxiao tonyxiao closed this Apr 14, 2015
@tonyxiao
Copy link
Author

Actually let me reopen this for discussion for now, and once we are on the same page I can submit separate PRs if needed.

@tonyxiao tonyxiao reopened this Apr 14, 2015
@tonyxiao
Copy link
Author

I'm actually no longer using METMethodInvocation in any client code, I have indeed found that callback based approach is more explicit and direct. However I am still using delegation to manage subscription readiness. Unlike method calls which are transient, subscriptions are long lasting objects that have lifetime throughout the application. Often multiple subscriptions has to be ready before user can be shown a particular screen. Now this could be accomplished with call back blocks, but in practice it when you implement a large # of callback blocks at the same time (some for subscription, others for login/out, etc), it makes the setup code super long and difficult to read, and you end up invoking a separate function from the body of the callback block anyways to keep functions short and clean.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants