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

Remove Public Promise #709

Draft
wants to merge 16 commits into
base: master
from

Conversation

@designatednerd
Copy link
Contributor

commented Aug 13, 2019

NOTE: Leaving this as a draft because I'd like @martijnwalraven to take a look at this when he's back from vacation, and I'd love to hear any other feedback in the meantime.

This PR addresses #382. The promises Martijn created were originally intended to only be for framework-internal consumption, not for use by outside apps or SDKs. This is why we're using a custom Promise type rather than existing Promise libraries.

However, our Promise type conflicts with those existing libraries and can cause all sorts of weirdness and confusion when it's exposed publicly, and it also somewhat hamstrings future implementation choices (including removing Promises under the hood entirely).

For this PR, we're only removing the "public" aspect of promises and continuing to use them under the hood. This should give us more flexibility in the future, and make it easier to wrap these methods in their preferred promise library.

Note that this is going to be a BREAKING CHANGE!

@designatednerd designatednerd added this to the 0.16.0 milestone Aug 16, 2019

@designatednerd designatednerd force-pushed the rm/public-promise branch from b3bc776 to bef0125 Aug 27, 2019

@martijnwalraven
Copy link
Contributor

left a comment

Glad to get rid of the publicly exposed Promise implementation! It's a breaking change, but I think it's worth it and it shouldn't impact too many people.

I think the code could be simplified with generic wrapper methods to convert methods from/to the use of promises, but that's probably not worth the trouble.

@@ -258,3 +301,48 @@ public final class ApolloStore {
}
}
}

internal extension NormalizedCache {

This comment has been minimized.

Copy link
@martijnwalraven

martijnwalraven Sep 3, 2019

Contributor

It's probably not worth the trouble, but I wonder if we could use something like Node's promisify to avoid writing these wrapper methods manually.

This comment has been minimized.

Copy link
@designatednerd

designatednerd Sep 3, 2019

Author Contributor

Since I'd eventually like to remove our promises system I'm not entirely convinced making them easier to create is worth it. 😆

}.andThen {
fulfill(())
}
guard let completion = completion else {

This comment has been minimized.

Copy link
@martijnwalraven

martijnwalraven Sep 3, 2019

Contributor

Maybe apollo_performAsyncIfNeeded could be extended with a guard for an optional callback?

public func withinReadTransaction<T>(_ body: @escaping (ReadTransaction) throws -> T,
callbackQueue: DispatchQueue? = nil,
completion: ((Result<T, Error>) -> Void)? = nil) {
_ = self.withinReadTransactionPromise {

This comment has been minimized.

Copy link
@martijnwalraven

martijnwalraven Sep 3, 2019

Contributor

This may not be worth it, but maybe a generic wrapper method like Node's callbackify could be used to abstract this conversion.

@designatednerd designatednerd force-pushed the rm/public-promise branch from bef0125 to eaac434 Sep 4, 2019

import Foundation

public extension DispatchQueue {

This comment has been minimized.

Copy link
@koenpunt

koenpunt Sep 6, 2019

Does this extension need to be public?

This comment has been minimized.

Copy link
@designatednerd

designatednerd Sep 6, 2019

Author Contributor

Yes, in order for it to be usable in the ApolloSQLite framework.

@designatednerd designatednerd force-pushed the rm/public-promise branch from 5a5494e to 3b8b444 Sep 10, 2019

@designatednerd designatednerd force-pushed the rm/public-promise branch from 3b8b444 to 9f914e1 Sep 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.