snipsco / Postal

A Swift framework for working with emails
MIT License
652 stars 82 forks source link

Change Postal completion handlers queue #34

Open klefevre opened 7 years ago

klefevre commented 7 years ago

Today we dispatch results on the main thread because we assume that Postal is mostly used for UI purpose which is not necessarily true.

It's just an idea but I think we should remove this behaviour because if we use Postal for processing purpose we may not want Postal to enqueue on the main queue (even if here we are talking about very lightweight messages). Users might want to save some computations from the main-thread before actually displaying something to the UI.

So I would propose to drop this behaviour. As an example NSURLSession doesn't dispatch on the main thread even if the most common usage of it is to display something fetched from internet on the UI. And if we think about it. Postal and NSURLSession do, in a sense, the same kind of work...

As an alternative, we could add an option to the Postal instance or adding a parameter to methods that have a completion handler to let the choice to dispatch results on the main thread or not.

jeremiegirault commented 7 years ago

Like NSURLSession, I think we should create a "Configuration". The default configuration would setup the dispatch on main queue. A custom configuration could allow the client to define its own dispatch queue. What do you think ?

Should we consider using an output OperationQueue like NSURLSession does or stay on DispatchQueue ? We will have to keep an internal serial DispatchQueue because libetpan is not thread safe at all, but we can propose the user to define an output OperationQueue.

klefevre commented 7 years ago

I like your idea of a "Configuration" struct but we already have a "Configuration" parameter in the initializer related to the IMAP session. I don't like the idea of putting things like these into it.

I would suggest instead adding a simple parameter in the initializer. It would look like this:

open class Postal {
     public init(configuration: Configuration, completionQueue: OperationQueue = .main)
jeremiegirault commented 7 years ago

Disagree, in order to future proof the API

We should indeed have a ProviderConfiguration. We should have another struct for PostalConnection Options (separation of concerns). We are in breaking mode so we can allow ourselves to change this, But let's assume that other options will emerge.

klefevre commented 7 years ago

Ok fair enough. Have you an idea of what should be added in this new structure aside the completion queue ?

jeremiegirault commented 7 years ago

At the moment the timeout parameter is provided "ad-hoc" in the connect func : https://github.com/snipsco/Postal/blob/master/Postal/Postal.swift#L65 All of these tweaks Which are note provider specific could go there.

klefevre commented 7 years ago

To answer your question. OperationQueue didn't seems to have change a lot for a while unlike DispatchQueue so in my opinion DispatchQueue would be better. Moreover I think that most of developers use it in a first place and switch to OperationQueue for a very specific case.

However we have to remember that we used an OperationQueue for libetpan because of a bug of dispatch_async that was sending some messages to the main thread..

EDIT: Personally I would like to switch to a full DispatchQueue implementation. WDYT?