Real-Serious-Games / C-Sharp-Promise

Promises library for C# for management of asynchronous operations.
MIT License
1.19k stars 149 forks source link

Delegate 'Action' does not take one arguments. #86

Closed davvit closed 5 years ago

davvit commented 5 years ago

this happens in .Net 4.6.1 console project.

promise.Done(links =>              // Receives the flattened collection of links from all pages at once.
            {
                foreach (var link in links) {
                    Console.WriteLine(link);
                }
            });

image

jdnichollsc commented 5 years ago

@davvit you need to attach all your code

RoryDungan commented 5 years ago

It looks like you are not returning a value from the promise before your call to .Done(...). This means that at the stage where Done is called, there is no longer a value being passed by the promise. You can solve this by either just using the version of Done that doesn't take any arguments or making sure that the promise you're calling Done on is of type IPromise<T>, not just IPromise, by making sure that any previous call to .Then(...) returns a value..

davvit commented 5 years ago

@jdnichollsc @RoryDungan 'my code' is also exactly similar to the project documentation code linked below https://github.com/Real-Serious-Games/C-Sharp-Promise#combining-multiple-async-operations

@RoryDungan not returning a value from a previous promise should not stop me from expecting a value in a done or in another promise chain. And this is described here. https://github.com/Real-Serious-Games/C-Sharp-Promise#chaining-synchronous-actions-that-have-no-result

I used Done() under the assumption its a type of Promise with some special behavior... that's why it made sense to pass 'links' which was the result of a promise chain...just as shown in the doc.

This is from the doc as well.

Terminating a Promise chain using Done: promise.Then(v => Something()) .Then(v => SomethingElse()) .Then(v => AnotherThing()) .Done(); // <--- Terminate the pipeline and propagate unhandled exceptions. To use the Done you must apply the following rule: When you get to the end of a chain of promises, you should either return the last promise or end the chain by calling Done.

As it is now it seems Done() is not to be used to do work on the result of promise chain.

I have not tested this code back again in my project; as I can do my work without calling Done(). I was using it just like another then...so I just did that. I just thought it was weird its not type of Promise.

RoryDungan commented 5 years ago

Ah it looks like that was an error in our documentation. In past versions you used to be able to "skip" over a Then that yielded no result and use the result later, but that functionality has since been removed (I think in v2.0) to bring the library more in line with the way JavaScript promises work.

I will double check and update the docs accordingly. The code in the first example you linked https://github.com/Real-Serious-Games/C-Sharp-Promise#combining-multiple-async-operations should still work because the .Done(...) function uses the value returned from the previous .Then().

RoryDungan commented 5 years ago

Okay I've updated the readme. If you want to use the value of a previous operation inside your done you will need to explicitly pass it though:

    var promise = promise
        .Then(result => SomeAsyncOperation(result))     // Chain an async operation.
        .Then(result => {
            Console.WriteLine(result));
            return result; // explicitly pass result through
        })
        .Done(result => ...);  // now we can use the result because it was returned by the previous promise.
davvit commented 5 years ago

i can understand this argument and flow.

But I think the proper implementation would be to always be able to result in a value when using Done - regardless if any of the promise chains returned value.

this is from JS doc at https://www.promisejs.org/

Awaiting a promise
In order to use a promise, we must somehow be able to wait for it to be fulfilled or rejected. The way to do this is using promise.done

function readJSON(filename){
  return new Promise(function (fulfill, reject){
    readFile(filename, 'utf8').done(function (res){
      try {
        fulfill(JSON.parse(res));
      } catch (ex) {
        reject(ex);
      }
    }, reject);
  });
}

in JS, regardless a value is returned or not; a promise is rejected or resolved; we can do 'result' and check if its null or not OR do try catch block as the example.

RoryDungan commented 5 years ago

The reason this works in JS is because in JS you can give any callback however many arguments you like and ones that are unused will be set to undefined. You can test this if you run the following code in Node or a browser:

Promise.resolve(5)
    .then(v => console.log(v)) // log the value but don't return anything in the callback
    .then(v => console.log(b)) // at this stage "v" should be undefined because the previous promise did not return a value

// Should print:
// 5
// undefined

This is a feature of JavaScript's weak typing. However, in C# we can tell that the second then callback will never have a value passed to it because the promise it's chained after doesn't return a value. Because it will never be used, we make it so that you have to use the version of then or done that doesn't pass an argument to its callback. Even though you aren't passing a value, the callback will still be invoked (with no arguments) and the promise can still be resolved or rejected.

This is actually closer to JavaScript promises then our previous implementation that remembered the last value from a value-returning promise and let you use it later down the chain, because that feature is not supported by standard JS promises.