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

Tame #1942

Closed
wants to merge 247 commits into from
Closed

Tame #1942

wants to merge 247 commits into from

Conversation

maxtaco
Copy link
Contributor

@maxtaco maxtaco commented Dec 17, 2011

Here's a first stab at a tame pull request, comments welcome. See TAME.md for more details.

@rtsuk
Copy link

rtsuk commented Dec 17, 2011

I've got a nodejs script that uses child_process.exec() extensively that would be a excellent test case. I'll port it over to this approach on Monday.

@paulmillr
Copy link

Curious about readability of real-world async coffeescript code.

Could you (or someone), please, show await / defer version of this function?

  compile: (files, callback) ->
    async.sortBy files, @sort, (error, sorted) =>
      return @logError error if error?
      async.map sorted, @map, (error, mapped) =>
        return @logError error if error?
        async.reduce mapped, null, @reduce, (error, reduced) =>
          return @logError error if error?
          @write reduced, (error) =>
            return @logError error if error?
            @log()
            callback @getClassName()

@maxtaco
Copy link
Contributor Author

maxtaco commented Dec 17, 2011

Just doing a simple translation would be the following, however, it seems as if you're leaking callback in the case of errors, no?

compile: (files, callback) ->
   await async.sortBy files, @sort, defer error, sorted
   return @logError error if error?
   await async.map sorted, @map, defer error, mapped
   return @logError error if error?
   await async.reduce mapped, null, @reduce, defer error, reduced
   return @logError error if error?
   await @write reduced, defer error
   return @logError error if error?
   @log()
   callback @getClassName()

@ryanflorence
Copy link

This looks amazing.

This new (awesome) syntax clearly doesn't stick very well with CoffeeScript's general behavior of having the JavaScript really similar to the CoffeeScript. The transpiled JavaScript is wildly different looking code--nobody would ever write their JavaScript that way.

Here is the translated output (slightly hand-edited for clarity):

Are those comments like // await block f4() part of the output? I think they should be.

Would love some brainstorming on making the output a little more human readable. Similar to how things get stored in _ref in a comprehension, maybe assign all those function expressions and pass them around?

_await_f4 = function () {}
_continue_f4 = function () {}
(function() {
  (_await_f4())
  (_continue_f4())
  ... etc
});

Awesome work, can't wait to try this out.

@paulmillr
Copy link

Looks nice! 👍 from me.

Added:
I would be even more happy if there was some DRY method to handle errors.

@stephank
Copy link

TAME.md should've been a link, so here you go.

@paulmillr: I've only read TAME.md, but I figure it'd look like:

compile: (files, autocb) ->
  await async.sortBy files, @sort, defer(error, sorted)
  return @logError error if error?
  await async.map sorted, @map, defer(error, mapped)
  return @logError error if error?
  await async.reduce mapped, null, @reduce, defer(error, reduced)
  return @logError error if error?
  await @write reduced, defer(error)
  return @logError error if error?
  @log()
  return @getClassName()

There's a slight difference in behavior, though. Every return invokes the callback for compile, while the original code just did it once at the end. In case of an error, the callback receives the return value of logError. But that's a side-effect of autocb and can be avoided by simply not using it.

This has left me with the question of how to use autocb and a callback with multiple arguments. It's very common in Node.js for callbacks to have (error, result) style callbacks.

Also, I figure any throw after the first await is no longer caught by a try-catch block outside of the function, much like 'regular' async code we have now? (e.g., in @paulmillr's example, a try-catch around the call to compile.)

@twojcik
Copy link

twojcik commented Dec 17, 2011

+1

@maxtaco
Copy link
Contributor Author

maxtaco commented Dec 17, 2011

@stephank good point about autocb only taking zero or one parameter. The current coffee grammar isn't happy with return(a,b). Something like return [a,b] means something else. I was hoping to avoid any new keywords. Any other ideas? I'm stumped. Thanks!

@maxtaco
Copy link
Contributor Author

maxtaco commented Dec 17, 2011

@rpflorence thanks the for comments. At the very least regular coffee code without await will compile as before. Of course that's not a great answer. Loops are definitely going to look different. The tamejs translation uses a fair number of temporaries, sort of like your suggestion, but that makes things look even worse, in my opinion.

@showell
Copy link

showell commented Dec 17, 2011

My vote is to keep this as a fork until CS gets line number support. I understand the use case of making async code nicer to write in JS, but it seems like a lot of extra complexity for the compiler. I wish the translation continued to happen at the JS part of the stack, and that the only CS hooks were more generic, like the ability to define a custom prefix for statements, or maybe some sort of generic annotation feature.

The reason that I'd postpone this until line number support is that I think the CS compiler could benefit from a little bit of rewriting, and the tame features are just gonna make it that much more difficult to do a rewrite.

The rewrite that I'd like to see on CS is make the parsing stage a little more generic about building an AST, and then let the code generation piece do more of the reassembly type stuff, such as marrying ifs with else.

In general, regardless of line number support, this is obviously a pretty major change to CS, and it seems like we should be conservative about pulling it in.

I'm happy to elaborate on the "generic hooks/annotation" idea if that has any resonance with other folks.

@aaronshaf
Copy link

+1 !!!!!

@michaelficarra
Copy link
Collaborator

I want to echo a few of @showell's comments: we must be very cautious with this very large change, and the current relatively small compiler very badly needs a cleanup or a rewrite.

I'm going to go through the diff now, paying attention only to proper style or potential bugs. Before I do that, though, @maxtaco: does the syntax used here have any advantages over the syntax I proposed in #1710? I think the syntax from #1710 is more approachable and natural.

await dns.resolve host, "A", defer(err, ip)
msg = if err then "ERROR! #{err}" else "#{host} -> #{ip}"
console.log msg
cb()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do cb

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure func() is more widely used than do func, including within the compiler... see e.g. http://coffeescript.org/documentation/docs/lexer.html.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do is still a new syntax for CS, so it's reasonable that it wouldn't be all over the compiler yet. do should be used when you're not directly using the value of a function invocation, like when using a function for its side effects. @jashkenas: do you agree? Though this one's kind of different because it's implicitly part of a return, so either way would be acceptable in my eyes.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. I still have faint hopes of being able to axe do eventually... At the moment, it exists as a necessary evil to avoid the ((arg)->)(arg) pattern, and the ability for it to invoke an arbitrary expression is sort of a consequence of consistency, but I don't think it would be considered good style. This isn't a strong proscription -- just a note for patches against the compiler.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do cb

I've been using do exactly as @michaelficarra describes ('not directly using the value of a function invocation'), especially when an invocation appears at the end of a method, and it really clears up my code in my eyes.

@maxtaco
Copy link
Contributor Author

maxtaco commented Dec 17, 2011

@michaelficarra I like the syntax in #1710, and I'd be open to changing the syntax to make it more Coffee-flavored. But I don't see how it would work with something like a Rendezvous or a Pipeliner in the tame_advanced test file. Both cases have a defer outside an await.

Thanks for taking such a careful read through the code.

@michaelficarra or @jashkenas, should I still make these changes/fixes in my branch?

@jashkenas
Copy link
Owner

Yes, if you continue to make changes and fixes in this branch, the pull request will continue to be updated.

@michaelficarra
Copy link
Collaborator

@maxtaco:

I'd be open to changing the syntax to make it more Coffee-flavored. But I don't see how it would work with something like [...]

I'll have to look into it once I'm more familiar with the contents of the pull request.

should I still make these changes/fixes in my branch?

Go ahead and make the changes that I've mentioned so far so that I can delete all those comments and then send you a pull request for anything that's left.

I did want to note that the autocb magics repulsed me on my first read-through. I don't think magic behaviour like that is ever appropriate. We should look into possibly using some sigil instead (I know, I know, Perlisms...) to annotate the fact that an argument behaves as an autocb.

@yozlet
Copy link

yozlet commented Dec 17, 2011

Another +1 to the comment from @showell. I'm just a CS user, not someone who's worked with the source. While I would dearly love yield/await in CS (trust me, I'm desperate for something like this), it's more important to my day-to-day development work that CS code be easily debuggable. The harder it is to trace JS errors back to the originating CS code, the less useful CS becomes.

@showell : Would these generic hooks be anything like the long-dormant CS extension ideas? See issue #257. I'd love to hear whether CS insiders are open to waking this one up again.

@goatslacker
Copy link

What I currently like most about CoffeeScript is the comprehensible JavaScript it outputs. I realize using await and defer is optional, but I'd still like to see JavaScript more consistent with what I'm used to seeing from CoffeeScript.

Overall I believe the complexity this introduces outweighs what it's trying to fix which isn't necessarily broken. Is there a cheaper price we can pay for this sugar?

@maxtaco
Copy link
Contributor Author

maxtaco commented Jan 29, 2012

@devongovett, I've enabled issue reporting, didn't realize it wasn't on by default. Thanks for that bug report, I'll puzzle it over.

@superjoe30, my original thought on nested awaits is that they should cause a compiler error. I don't think they're very useful and you can always introduce intermediary anonymous functions to be explicit about what's going on. But your example can make sense. Issue the first foo and the second in parallel, and issue the third after the second completes. Exit the await once the first and third have completed. This seems weird to me, but you're right, it ought to work or complain that it can't.

I'll make two issues over in the branch.

@bigeasy
Copy link

bigeasy commented Jan 30, 2012

-1 I've embraced Streamline + CoffeeScript. I prefer that syntax. Adding this to the core of CoffeeScript would be a such a disappointment.

@weepy
Copy link

weepy commented Jan 30, 2012

Is there not something more basic/fundemental that can be added to CoffeeScript that would enable users to build async frameworks upon? It seems to me very much again CoffeeScript to include libs like tame/streamline in the core. I would have thought/hoped that there would be some kind syntax that could be added to enable them though ?

@bigeasy
Copy link

bigeasy commented Jan 30, 2012

@weepy To clarify, I'm not arguing for Streamline over Tame. I'm saying that having geared by development to Streamline, with CoffeeScript, and builds that take care of everything, I don't want to see Tame become the default because someone petitioned to have it rolled into the core.

@weepy
Copy link

weepy commented Jan 30, 2012

  • deleted - syntax highlighting wasn't working

@weepy
Copy link

weepy commented Jan 30, 2012

  • yes I got that ^_^

and to be clear I wasn't arguing for either to be in core - just for
something more 'fundemental'. E.g in Kaffeine, a function with !
allows it to be called async:

res.send({
  count: User.count!,
  puzzles: User.puzzles!("today")
})
=>
User.count(function(_0) { User.puzzles("today", function(_1) { res.send({
    count: _0,
    puzzles: _1
  }) }) })

or

sum = add!(3, 4).length
=>
add(3, 4, function(_0) { sum = _0.length })

Now this is pretty neat, and allows various expression to be used in
as though they were sync. However it is only fairly simple sugar and
won't properly handle parellel style like:

for i, val in A
  $.getJSON! val

@andrewrk
Copy link

I'm starting to see this from the perspective that Iced should be a library. I don't think you'd even have to change the syntax that much, really.

@jashkenas
Copy link
Owner

Hey folks. Just ran across some prior art for this kind of thing -- which I don't recall seeing before -- StratifiedJS' take:

https://github.com/onilabs/coffee-script

... if you're interested in this ticket, it's worth a look.

@satyr
Copy link
Collaborator

satyr commented Feb 1, 2012

which I don't recall seeing before

https://github.com/jashkenas/coffee-script/issues/search?q=stratified

@maxtaco
Copy link
Contributor Author

maxtaco commented Feb 1, 2012

Stratified definitely predated the original tamejs stuff by some months. I think it's doing CPS conversions and solving the callback pyramid problem. This is a taste issue, but I prefer the iced syntax for two reasons: (1) fewer keywords/syntax changes; (2) callbacks are represented explicitly. That callbacks are represented explicitly gives the ability to get interesting control flow patterns that Stratified accomplishes with further keywords. Take for instance the iced.Rendezvous class which allows for the programmer to act after the first computation completes; or the icedlib.Pipeliner class which makes it easy to set up a windowed pipeline of computations; or the icedlib.timeout class which allows the programmer to "split" an arbitrary events into two racing events, allowing for a simple implementation of timeouts. In all cases, the libraries are explicitly playing around with callbacks, so the language internals don't have to.

@ajoslin
Copy link

ajoslin commented Mar 5, 2012

+1, I really like this.

@wamatt
Copy link

wamatt commented Sep 9, 2012

+1

No comments for six months, weird. Anyway I really feel Iced is a rather elegant and exciting addition to CS.

Being a superset, I don't think we'd loose much merging it back in, and we would gain a whole lot. CS is already a fragmentation in the JS ecosystem and would be great to unify the talents in the community. Additionally, having another fork further fragments the tooling, a real issue for users when deciding on suitability (which affects adoption).

my humble 2c :p

@vendethiel
Copy link
Collaborator

You may be interested in looking to #350

@rlidwka
Copy link

rlidwka commented Sep 25, 2012

CS is already a fragmentation in the JS ecosystem and would be great to unify the talents in the community.

CS should have modular system instead, so anybody could make extensions to CS that works with each other.

But I'd like to see tame here anyway.

@funny-falcon
Copy link

I used IcedCoffeeScript in prototype, and leave several await in production - all works great.
I found no any bug with Iced.
I remove await from linear-callback code cause CoffeScript give's already pretty syntax for callbacks.
But I leave await in cycles and conditions, cause they looks much prettier in this way.

@jorgebg
Copy link

jorgebg commented Nov 7, 2012

+1

1 similar comment
@dmitry-solomadin
Copy link

+1

@vendethiel
Copy link
Collaborator

You may be interested by #350

@paulmillr
Copy link

I was the second commenter in this thread. Iced coffeescript does not solve fundamental problem of async js code: composition.

The problem is absolutely effectively solved by promises:

async.sortBy(files, sort)
  .then (sorted) =>
    async.map sorted, @map
  .then (mapped) =>
    async.reduce mapped, null, reduce
  .then (reduced) =>
    @write reduced

You can use promises today and the result code is cute.

@funny-falcon
Copy link

How does this iced code is mapped to promizes?

    console.log "prepare"
    await
      for file in files
        fs.unlink file, defer()
    console.log "done"

@paulmillr
Copy link

@funny-falcon

Q.all(files.map (file) -> Q.nfcall fs.unlink, file).then(-> console.log 'done')
  1. You call fs.unlink with promise wrapper (Q.nfcall) on each file.
  2. You turn an array of promises to one promise with Q.all and then just use it simply.

Also, consider how your 4-line-coffee-code translates to 20 lines of js.

@funny-falcon
Copy link

@paulmillr
I know how my example translates to 20 lines of js - in a very genius way :-P

While I know, that iced-coffee-script will be certainly slower that your version,
I'll prefer it, cause it way more readable.

For simpler cases coffee-script already provides very pretty syntax for functions.
And for this rare case readability counts more than performance.

@vendethiel
Copy link
Collaborator

Closing for #2762

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.