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

Piped-promises for Android #65

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

Conversation

edeckers
Copy link

@edeckers edeckers commented Sep 18, 2015

I added piped promises for Android as best as I could without modifying the non-Android code-base. This restriction unfortunately prohibited me from writing type-safe code (i.e. enforce use of Android-only promises when AndroidDeferredManager is used), so instead I had to fallback to enforcing the use of Android-promises by throwing exceptions whenever a non-Android promise is used in an Android-context.

Also I could not get the test-suite running in IntelliJ IDEA, so I am not sure if the single test I added even works, you will have to try that for yourself. I do use the code from this PR actively on a daily base though, which leads me to believe it works pretty well.


This change is Reviewable

@saturnism
Copy link
Member

hiya! could i trouble you to elaborate on the usage? also, I noticed many of the return types had changed from Promise to AndroidPromise. That's a pretty significant change that could break existing users.

@edeckers
Copy link
Author

You use it pretty much the same way as DonePipe, the difference being that you provide an execution scope, e.g.:

new AndroidDeferredManager()
  .when(new Callable<Foo>() {
      @Override
      public Foo call() throws Exception {
          ...
          return ...;
      }
  })
  ...
  .then(new AndroidDonePipe<Foo, Bar, Throwable, Void>() {
      @Override
      public AndroidPromise<Bar, Throwable, Void> pipeDone(Foo foo) {
          Bar bar = someAction(foo);
          ...
          return new AndroidDeferredObject<Bar, Throwable, Void>()
                  .resolve(bar);
      }

      @Override
      public AndroidExecutionScope getExecutionScope() {
          return AndroidExecutionScope.BACKGROUND;
      }
  })
  ...

Another solution would have been to just extend the AndroidDeferredManager with a then(DonePipe) method, but then you would not be able to provide an execution scope, which is a problem when you would like to do some network requests in the piped promise for example, since you're not allowed to execute those on the UI-thread.

As for the changed return types, when you're using promises on Android I think you should always use the Android-specific promises with execution scope; using the regular promises can result in 'weird' errors because they're always executed on the UI-thread. I tried to enforce this by only allowing Android-promises in the AndroidDeferredManager, but failed to do so without also making changes to the 'regular' code-base and unfortunately had to revert to throwing errors when someone uses a non-Android promise with AndroidDeferredManager.

So I guess the changed return types are not necessary for this PR, but you might consider keeping them anyway, because it helps users realize that Android-promises are the preferred choice when using AndroidDeferredManager.

@saturnism saturnism added this to the 1.3 milestone Feb 27, 2016
@saturnism saturnism self-assigned this Feb 27, 2016
@@ -21,7 +21,7 @@
<version>1.2.5-SNAPSHOT</version>
<relativePath>../pom.xml</relativePath>
</parent>
<artifactId>jdeferred-android-aar</artifactId>
<artifactId>jdeferred-jdeferred-android-aar</artifactId>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hiya, could i trouble you to rename this back?

@aalmiray
Copy link
Contributor

ping

@saturnism saturnism removed this from the 1.3 milestone Oct 28, 2017
@saturnism saturnism removed their assignment Oct 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants