search

jakejs / jake

JavaScript build tool, similar to Make or Rake. Built to work with Node.js.
http://jakejs.com
Apache License 2.0
1.97k stars 190 forks source link

Docs don't mention anything about promise resolution #270

Open lawrencejones opened 10 years ago

lawrencejones commented 10 years ago

Well this is unusual. You guys appear to have built a killer feature, and then written absolutely nowhere that you support it.

I've been trying to figure out why my async tasks would complete despite no call to complete. It turns out, in doing something like this...

  desc 'Setup git hooks by symlinking .git/hooks dir'
  task 'hooks', ->

    Chain\
    ( [ 'rm -rf ./.git/hooks'
      , 'Failed to remove old git folder'
      , successCheck: (-> !fs.existsSync './.git/hooks') ]
      [ 'ln -s ../hooks ./.git/hooks'
      , 'Failed to symlink ./.git/hooks -> ./hooks' ]
    )
    .title 'Symlinking git hooks'
    .success 'Successfully symlinked ./.git/hooks -> ./hooks'
    .run()

...where Chain(...).run() will yield a promise, Jake appears to be detecting the promise format and resolving the task normally.

Don't get me wrong, this is cool. I really like it. But your documentation mentions nothing about this feature? If you then call complete in these functions, you can get really screwed up, with tasks triggering each other due to a duplicate complete call.

If I get the time I can make a PR with changed docs, but just wanted to confirm here that this is desired behaviour.

An example of how this can create unpredictable behaviour, imagining this appended after the .run()...

.then ->
  setTimeout (-> complete()), 1000

...the setTimeout will be ignored, as the promise has been resolved (albeit with undefined) and then a duplicate call to complete can screw up the execution chain.

jaawerth commented 7 years ago

It's definitely the desired behavior - I was actually evaluating adopting jake as a tool and disappointed enough that it lacked this kind of Promise support that I was considering a PR to add the functionality. But first, I searched the repository for Promise and was surprised to see that not only was the functionality there, but there were also tests for it.

+1 for adding this to the docs, it's a huge deal, particularly now that async/await is native. In my opinion, async tasks should get their own section, and the Promise support should get top billing over the other async task API.

mde commented 7 years ago

@jaawerth, I've invited you as a contributor. I'd love your help improving the docs here.

jaawerth commented 7 years ago

@mde Happy to! It might take a couple weeks to make time to go over everything, but I also noticed a few places that might benefit from a code PR to even out the promise support (for example, rules don't appear to benefit from the same Promise support as regular tasks - or, at least, a rejected promise goes unhandled rather than leading to jake aborting the task).

mde commented 7 years ago

That would be awesome. I would love to get that Promise support more unified. (Not to mention async and await.) Let me know how I can help.