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

Progress propagation #58

Closed
mjbeauregard opened this issue Feb 29, 2016 · 4 comments
Closed

Progress propagation #58

mjbeauregard opened this issue Feb 29, 2016 · 4 comments

Comments

@mjbeauregard
Copy link

I am having problems getting progress events to propagate up the chain of tasks.

I have a lower level String task that will generate progress. Then, upon success I want to convert that String value to in Int if possible or an error otherwise. I expect progress handlers on both tasks to be called whenever progress changes on the lower level task. Is this a valid expectation?

To be clear, consider the following test (using Nimble) that always fails where indicated below:

class ProgressTest: QuickSpec {
    override func spec() {
        var stringTask: Task<Float, String, NSError>!
        var stringTaskProgressHandler: (Float ->Void)?
        var stringTaskProgressValue: Float?

        var intTask: Task<Float, Int, NSError>!
        var intTaskProgressValue: Float?

        beforeEach {
            stringTask = Task<Float, String, NSError> { progress, fulfill, reject, configure in
                stringTaskProgressHandler = progress
            }.progress { oldValue, newValue in
                stringTaskProgressValue = newValue
            }

            intTask = stringTask.success { stringValue -> Task<Float, Int, NSError> in
                if let intValue = Int(stringValue) {
                    return Task(value: intValue)
                } else {
                    return Task(error: NSError(domain: "test", code: 123, userInfo: nil))
                }
            }.progress { oldValue, newValue in
                intTaskProgressValue = newValue
            }
        }

        describe("when string task has progress") {
            beforeEach {
                stringTaskProgressHandler?(0.5)
            }

            it("string task progress should fire") {
                expect(stringTaskProgressValue).toEventually(equal(0.5))
            }
            it("should propagate progress to intTask") {
                // ************ This test fails! ************
                expect(intTaskProgressValue).toEventually(equal(0.5))
            }
        }
    }
}

Thoughts?

@mjbeauregard
Copy link
Author

Another person on my project had a look at this test and noted that the test will work if I add the progress handler before adding the success handler that converts the value from a String to Int. I don't think this is a solution for me though since, in reality, the code that produces the original String task exists independently from the code that wants to take that successful task and convert its value to Int.

I rewrote my test code from above in order to get it to chain the success, failure and progress handlers the way I was expecting so that the test now passes. It accomplishes this by wrapping the original task in another that performs the conversion. This seems really verbose and almost certainly indicates that I'm missing a key concept here:

class ProgressTest: QuickSpec {
    override func spec() {
        var stringTask: Task<Float, String, NSError>!
        var stringTaskProgressHandler: (Float ->Void)?
        var stringTaskProgressValue: Float?

        var intTask: Task<Float, Int, NSError>!
        var intTaskProgressValue: Float?

        beforeEach {
            stringTask = Task<Float, String, NSError> { progress, fulfill, reject, configure in
                stringTaskProgressHandler = progress
            }.progress { oldValue, newValue in
                stringTaskProgressValue = newValue
            }

            // wrap the original task instead of chaining to ensure progress is propagated
            intTask = Task<Float, Int, NSError> { progress, fulfill, reject, configure in
                stringTask.progress {
                    progress($1)
                }
                stringTask.on(success: { stringValue in
                    if let intValue = Int(stringValue) {
                        fulfill(intValue)
                    } else {
                        reject(NSError(domain: "test", code: 123, userInfo: nil))
                    }
                }, failure: { error, wasCancelled in
                    if let error = error {
                        reject(error)
                    } else {
                        // what do I do here?
                    }
                })
            }.progress { oldValue, newValue in
                intTaskProgressValue = newValue
            }
        }

        describe("when string task has progress") {
            beforeEach {
                stringTaskProgressHandler?(0.5)
            }

            it("string task progress should fire") {
                expect(stringTaskProgressValue).toEventually(equal(0.5))
            }
            it("should propagate progress to intTask") {
                // ************ This test now passes! ************
                expect(intTaskProgressValue).toEventually(equal(0.5))
            }
        }
    }
}

@inamiy
Copy link
Member

inamiy commented Mar 1, 2016

In your first example,

let task = Task<Float, String, NSError> { progress, fulfill, reject, configure in
    stringTaskProgressHandler = progress
}

is a task that never fulfills (succeeds) or rejects (fails).
That means, task.success { /* this will never be called */ }.

Also, if you method-chain using then/success/failure followed by next progress, you are going to observe a new task's progress instead, i.e. someTask.success { /* return newTask */ }.progress { /* observes newTask's progress, not someTask's */ }.

@mjbeauregard
Copy link
Author

Your explanation fits the behaviour I am seeing, but I'm still surprised that it works this way. I understand why it works this way, but my intuition was that there would be an easy way to propagate progress (and cancellation) along the chain in order to not lose track of progress being emitted by the original task. I think my approach for composing tasks in this way simply doesn't fit the intent of the library. Anyway, thanks for clarifying the expected behaviour.

@inamiy
Copy link
Member

inamiy commented Mar 3, 2016

This is how Promises/Future library normally works.
When then/success/failure/flatMap (or any similar funcs) is called, it is normally wrapping a new task inside, and it should no longer represent the very first task anymore.

It is good idea to check the difference between task.progress { ... }.success { ... }.progress { ... } and task.progress { ... }.progress { ... }.success { ... }. Hope this helps!

@inamiy inamiy closed this as completed Mar 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants