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

Future.finally #71

Open
TheLudd opened this issue Sep 1, 2015 · 9 comments
Open

Future.finally #71

TheLudd opened this issue Sep 1, 2015 · 9 comments

Comments

@TheLudd
Copy link
Contributor

TheLudd commented Sep 1, 2015

Should there be an equivalence of a finally statement on futures? I recently used that with promises and I wondered how I would solve the same situation with futures. Folktale has a cleanup method but it is supplied with the constructor. To me that seems a bit weird. Should it not be called together with fork?

Thoughts?

@TheLudd
Copy link
Contributor Author

TheLudd commented Oct 13, 2015

No comments on this? I am using futures right now in production and feel as if I need this badly.

Could it be added or should the entire model be reconsidered as mentioned before?

Ping! @buzzdecafe @scott-christopher @davidchambers @CrossEye @DrBoolean

@davidchambers
Copy link
Member

Do you find yourself doing something like this?

fut.fork(err => {
           // do X...
           // do Z...
         },
         val => {
           // do Y...
           // do Z...
         });

Perhaps you could share a concrete example?

@TheLudd
Copy link
Contributor Author

TheLudd commented Oct 13, 2015

Yes that is is.
Most often, doing an async operation and before it starts I show a loader on screen. When the operation is finished I want to hide the loader both in the success and error case.

@davidchambers
Copy link
Member

What are you suggesting, exactly? Something like this?

fut.forkFinally(err => {
                  // do X...
                },
                val => {
                  // do Y...
                },
                () => {
                  // do Z...
                });

@CrossEye
Copy link
Member

I'm sorry, I've not paid much attention to R-F lately, nor at all to Futures. I will try to look at it this evening.

I really should, as I needed something like it lately and automatically reached for Folktale.Task. It would be nice to know if R-F has something that handles these needs.

@CrossEye
Copy link
Member

I would personally find it odd, and I would probably resist changing fork to do this. If there was another function, I suppose I would have no significant objection.

I can certainly understand the rationale, but it strikes me that this might well have to be in the user's control. Perhaps such a function should run before a resolve/reject. Perhaps it should run after one either way. But perhaps the user needs it to run before a reject, but only after a resolve. We certainly don't want to provide an API that offers those discrete choices, but the user can just embed a call to such a function inside her resolve/reject functions, so the ability is already there in fork, only slightly less convenient than might be hoped.

So neither 👍 nor 👎 from me.

But the good news is that I dropped this in where I was using Folktale's data.Task, and it worked without a hitch. This is the first time I've actually used R-F for any production code. It's nice to see it working well. Kudo's to those who've been putting in the effort here.

@TheLudd
Copy link
Contributor Author

TheLudd commented Oct 16, 2015

the user can just embed a call to such a function inside her resolve/reject functions

This is what I am doing right now and I really dislike it. It is duplication

The most important thing to me is the rationale as you say @CrossEye. I am open to suggestions on how to make it work. It does not have to be something called "finally", perhaps there are even better solutions.

But what about... (just thinking on the top of my head here)

Future.prototype.fork = function(reject, resolve) {
  try {
    this._fork(reject, resolve);
  } catch(e) {
    reject(e);
  }

  return {
    finally: function(fin) {
      if (fin) {
        fin()
      }
    }
  }
};

Did a quick test on my machine with this and it allows:

var f = getFuture()
f.fork(errFn, successFn).finally(finallyFn)

Works when the future succeeds, fails and does not supply a finally function.

@davidchambers
Copy link
Member

The idea seems reasonable, but it makes the type signature pretty messy:

Future#fork :: Future a ~> (Error -> ()) -> (a -> ()) -> { finally :: () -> () }

@CrossEye
Copy link
Member

I guess my concern is that I see nothing intrinsic to make us add an 'after' without a 'before'. In David's notation earlier,

fut.fork(err => {
           // do W...
           // do X...
           // do Z...
         },
         val => {
           // do W...
           // do Y...
           // do Z...
         });

Should this not be translated at

fut.forkFinally( () =>
                  // do W...
                },
                err => {
                  // do X...
                },
                val => {
                  // do Y...
                },
                () => {
                  // do Z...
                });

And that makes the type signature still worse:

Future#fork :: Future a => 
        { initially :: () -> () } -> (Error -> ()) -> (a -> ()) -> { finally :: () -> () }

If you don't want to do this, then why does 'after' get such precedence over 'before'?


var f = getFuture()
f.fork(errFn, successFn).finally(finallyFn)

I like this API, if we decide to go with a finally. But others have objected to anything that looks even vaguely like object methods in our public APIs before, so there might be pushback from some.

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

3 participants