google / promises

Promises is a modern framework that provides a synchronization construct for Swift and Objective-C.
Apache License 2.0
3.8k stars 294 forks source link

Problem with typing #89

Closed nateuni closed 5 years ago

nateuni commented 5 years ago

Firstly is there any IRC channel where I can go and ask questions? I have looked everywhere and have run out of options, so I will ask here if I may? I am refactoring previously working code over to having promises, and I am trying to understand what is going on and how to fix a typing error. To qualify, I am actually a Product Manager that studied CS that is working on my side project, so bare with me.

In my code: I have this execution flow of Promises as shown in this gist.

I can see that the nested promises are called but not recognised in the outer promise chain, as the Always executes before the nested promises do. And whilst executing the nested promises the: self.saveUserProfilePictureToFirestore(user: user, profileImage: image).then function never returns a promise from its closure, it just hangs. I believe this is because I need to make that initial call to the nested promises with a with return self.makeFacebookGraphRequest(accessToken: token).then. But when I do I get "Unexpected non-void return value in void function" error message on the last nested .then, I have tried all types of things and even got a mobile dev from work to try and help me and he too couldn't figure it out.

What am I missing here?

shoumikhin commented 5 years ago

Hi @nateuni,

Technically, we have a chat on Gitter. There's a corresponding badge about that in Readme. But you're more than welcome to post your questions as issues here.

Try to place a return statement in front of self.makeFacebookGraphRequest:

// ...
}.then { token, document in
  if document?.exists == false {
    return self.makeFacebookGraphRequest(accessToken: token).then { user in
      return self.downloadFacebookProfilePictureAsImage(userModel: user)
    }.then { user, image in
// ...

Ideally, you want to refactor that nested piece into a separate function to avoid nesting promises, then use it approximately like so:

// ...
}.then { token, document in
  return document?.exists ? self.dismiss(animated: true, completion: nil) : self.loginUser(with: token)
}.catch { error in
// ...

Thanks.

nateuni commented 5 years ago

So I made this changes:

}.then { (arg) in
    let (token, document) = arg
    return document?.exists ? self.dismiss(animated: true, completion: nil) : self.createNewUserAccount(token: token)
}.catch { error in

But I still get Unexpected non-void return value in void function. I have tried:

  1. Wrapping the dismiss in the function that returns a promise.
  2. Having nonPromise<Void> return types

Maybe my functions aren't setup correctly:

internal func createNewUserAccount(token: AccessToken) -> Promise<Void> {
        return Promise { fulfill, reject in
            return self.makeFacebookGraphRequest(accessToken: token).then { user in
                return self.downloadFacebookProfilePictureAsImage(userModel: user)
                }.then { user, image in
                    return self.saveUserProfilePictureToFirestore(user: user, profileImage: image)
                }.then { user in
                    return self.getProfilePictureUrlFromFirestoreForUser(user: user)
                }.then { user, imageUrl in
                    return self.createNewUserDocumentAsDictionary(user: user, avatarUrl: imageUrl)
                }.then { user, userDictionary in
                    return self.saveUserDocumentToFirestore(user: user, data: userDictionary)
                }.then {
                    fulfill(())
                    self.performSegue(withIdentifier: "TermsViewController", sender: self)
            }
        }
    }
shoumikhin commented 5 years ago

What do you get for the following code?

}.then { token, document in
  document?.exists
      ? wrap { self.dismiss(animated: true, completion: $0) }
      : self.createNewUserAccount(token: token)
}.catch { error in
private func createNewUserAccount(token: AccessToken) -> Promise<Void> {
  return makeFacebookGraphRequest(accessToken: token).then { user in
    self.downloadFacebookProfilePictureAsImage(userModel: user)
  }.then { user, image in
    self.saveUserProfilePictureToFirestore(user: user, profileImage: image)
  }.then { user in
    self.getProfilePictureUrlFromFirestoreForUser(user: user)
  }.then { user, imageUrl in
    self.createNewUserDocumentAsDictionary(user: user, avatarUrl: imageUrl)
  }.then { user, userDictionary in
    self.saveUserDocumentToFirestore(user: user, data: userDictionary)
  }.then {
    self.performSegue(withIdentifier: "TermsViewController", sender: self)
  }
}

Note: when a closure has only one statement, it can technically skip the return statement. If you still have compiler error, please, specify at which line it occurred.

nateuni commented 5 years ago

I will show both functions together as maybe it will give more context and I left the comments in about the return types.

    /// this is called when the user clicks the login button.
    private func flow(){
        //this gets valid facebook token for the app
        loginToFacebook().then {token in
            // returns Promise<AccessToken>
            // with the token we can sign into Firebase
            return self.signIntoFirebase(accessToken: token)
            // returns Promise<AccessToken>
        }.then { token in
            // we need to token to get the userId for the logged in user
            return self.getUserDocumentFromFirebase(userId: token.userId!)
            // returns Promise<(AccessToken, DocumentSnapshot?)>
        }.then { token, document in
            return document?.exists ? self.dismiss(animated: true, completion: nil) : self.createNewUserAccount(token: token)
        }.catch { error in
            self.log.error(error.localizedDescription)
        }.always {
            Dependency.dismissHud(self.hud, "Test title", "Test message text", 3)
        }
    }

    private func createNewUserAccount(token: AccessToken) -> Promise<Void> {
        return makeFacebookGraphRequest(accessToken: token).then { user in
            self.downloadFacebookProfilePictureAsImage(userModel: user)
            }.then { user, image in
                self.saveUserProfilePictureToFirestore(user: user, profileImage: image)
            }.then { user in
                self.getProfilePictureUrlFromFirestoreForUser(user: user)
            }.then { user, imageUrl in
                self.createNewUserDocumentAsDictionary(user: user, avatarUrl: imageUrl)
            }.then { user, userDictionary in
                self.saveUserDocumentToFirestore(user: user, data: userDictionary)
            }.then {
                self.performSegue(withIdentifier: "TermsViewController", sender: self)
        }
    }

Currently with that code I get this error: Closure tuple parameter '(AccessToken, DocumentSnapshot?)' does not support destructuring for the }.then { token, document in line. Xcode gives me an automated fix which results in the tuple being converted like:

}.then { (arg) in
     let (token, document) = arg
     return document?.exists ? self.dismiss(animated: true, completion: nil) : self.createNewUserAccount(token: token)
}.catch { error in

That gets rid of that error. When I do this change that you suggested:

}.then { token, document in
  document?.exists
      ? wrap { self.dismiss(animated: true, completion: $0) }
      : self.createNewUserAccount(token: token)
}.catch { error in

I get Result values in '? :' expression have mismatching types 'Promise<Any?>' and 'Promise<Void>' with self.createNewUserAccount(token: token) underlined.

So then I changed private func createNewUserAccount(token: AccessToken) -> Promise<Void> to private func createNewUserAccount(token: AccessToken) -> Promise<Any?> { just to get it to compile and run. Q: What should I actually do here?

I still get the same issue, the program always gets into the saveUserProfilePictureToFirestore() function, goes past the guards and calls the Firebase putData function (resulting image data gets saved to firebase) but it never returns from the closure.

Here is what I believe is the issue and what I don't get: The order of execution of the promises results in the .always { getting called whilst the promises in createNewUserAccount() haven't yet completed. So I believe that function call whilst still executing, just gets abandoned. The mobile dev from work tried some tricker with the putData function to force it to return the promise earlier and the same thing just happened in the next function.

Should I create a pending promise in this block:

}.then { token, document in
  document?.exists
      ? wrap { self.dismiss(animated: true, completion: $0) }
      : self.createNewUserAccount(token: token)
}

So the execution is forced to wait for it? If so what would that code look like in terms of creation, passing back and then resolving the pending promise in that block? I am asking as I have had so many issue and it has taken me so long that I like to be very explicit with my questions and solutions.

Thanks a lot again for taking the time btw, I really appreciate the help. I get the concepts but sometimes need a little help as I don't code for my day job.

shoumikhin commented 5 years ago

Would it help if you specify the return type explicitly? It should be Promise<Void>, but not Promise<Any?>.

}.then { (token, document) -> Promise<Void> in
  document?.exists
      ? wrap { self.dismiss(animated: true, completion: $0) }
      : self.createNewUserAccount(token: token)
}.catch { error in

I have no idea how saveUserDocumentToFirestore() does what it does under the hood. Perhaps it must be called on a background dispatch queue, because it blocks the caller (main) queue in that putData() you've mentioned?

Doesn't seem there's a need of a pending promise anywhere.

nateuni commented 5 years ago

Finally found the issues:

  1. Needed to fix the types and make sure the promise was being returned correctly.
  2. The Firebase Storage library was bumped and putData is behaving differently (rolling back solved the issue)
shoumikhin commented 5 years ago

Awesome!

Do you mind to share the code snippet with the fix? At least the one where you specified the return type.

Thanks.

nateuni commented 5 years ago

Sure. It was the Promise from the wrap that was the typing issue. And there needed to be a return for either result.

Here was the issue that I found for the Firebase Storage. Lesson: update yours pods.

As for the typing:

    private func flow(){
        loginToFacebook().then {token in
            return self.signIntoFirebase(accessToken: token)
        }.then { token in
            return self.getUserDocumentFromFirebase(userId: token.userId!)
        }.then { (arg) -> Promise<Void> in
            let (token, document) = arg
            if document?.exists == true {
                return wrap { self.dismiss(animated: true, completion: $0) }
            } else {
                return self.createNewUserAccount(token: token)
            }
        }.catch { error in
            self.log.error(error.localizedDescription)
        }.always {
            Dependency.dismissHud(self.hud, "", "", 1.5)
        }
    }

    private func createNewUserAccount(token: AccessToken) -> Promise<Void> {
        hud.textLabel.text = "Creating new user account"
        self.log.info("<>")
        return makeFacebookGraphRequest(accessToken: token).then { user in
            self.downloadFacebookProfilePictureAsImage(userModel: user)
        }.then { user, image in
            self.saveUserProfilePictureToFirestore(user: user, profileImage: image)
        }.then { user in
            self.getProfilePictureUrlFromFirestoreForUser(user: user)
        }.then { user, imageUrl in
            self.createNewUserDocumentAsDictionary(user: user, avatarUrl: imageUrl)
        }.then { user, userDictionary in
            self.saveUserDocumentToFirestore(user: user, data: userDictionary)
        }.then {
            self.performSegue(withIdentifier: "TermsViewController", sender: self)
        }
    }