Skip to content
This repository has been archived by the owner on Dec 7, 2020. It is now read-only.

Comments on Documentation #82

Open
richb-hanover opened this issue Feb 5, 2016 · 10 comments
Open

Comments on Documentation #82

richb-hanover opened this issue Feb 5, 2016 · 10 comments

Comments

@richb-hanover
Copy link

Update: Changed subject to reflect the current focus.

I tried out the example at https://github.com/getify/asynquence/blob/master/contrib/README.md#errfcb-plugin that talks about sq.errfcb() plugin. When I execute the code, the parameter passed to .then() doesn't seem to have the desired data. (That's the data which would have been passed as the second parameter of the error-first callback). How is it made available?

// first install with "npm install asynquence"
var ASQ = require("asynquence-contrib");
fs = require("fs");

// Node.js: fs.readFile(..) wrapper
function readFile(filename) {
  // setup an empty sequence (much like an empty
  // promise)
  var sq = ASQ();

  // call Node.js' `fs.readFile(..), but pass in
  // an error-first callback that is automatically
  // wired into a sequence
  fs.readFile( filename, 'utf8', sq.errfcb() );

  // now, return our sequence/promise
  return sq;
}

readFile("/Users/richb/meaningoflife.txt")
  .then((data)  =>
    { console.log("Read fine...", data) })
  .or((err) =>
    { console.log("Error", err) })

Update: added 'utf8' parameter to fs.readFile() so that resulting data is a string, not a buffer.

@richb-hanover
Copy link
Author

Answered my own question. Parameters to .then() must be (done, data)...

//...
readFile("/Users/richb/meaningoflife.txt")
  .then((done, data)  =>
    { console.log("Read fine...", data) })
  .or((err) =>
    { console.log("Error", err) })

It would be good to expand the example to include these lines that consume the data/err results.

@getify
Copy link
Owner

getify commented Feb 5, 2016

In ASQ, the then(..) function assumes the following signature for its callbacks:

( triggerDoneCallback, msg1, msg2, ... )

If you want it to behave like a promise#then(..) method -- leave off the triggerDoneCallback and since it only accepts a single arg, all those msg1, msg2, etc have to wrapped into a single array -- then use the pThen(..) method from the pThen contrib plugin.

Alternatively, since you're in this example doing a synchronous step (that is, not needing to return for chaining a sub-sequence) like console.log(..), just use .val(..) instead of .then(..).

To summarize, your 3 options are:

  1. .then( (_, data) => .. )
  2. .pThen( data => .. )
  3. .val( data => .. ) (synchronous steps only)

@getify
Copy link
Owner

getify commented Feb 5, 2016

heh, cross posting. :)

@richb-hanover
Copy link
Author

Thanks for the speedy response. I'm new to promises/ASQ/etc. and don't fully have a sense of how to "speak in Promises" versus how to "speak in asynquence".

Here are a couple comments about things that made it hard for me as a newcomer to get started.

  • For those who are trying to follow the basic pattern (without fancy things such as 2) or 3) above), it would help for the doc's to be a bit more prescriptive, and maybe have a fully-worked example.

  • As I noted above, it would have helped me if those final lines of the example showed how to consume the err and data from the original callback.

  • It would be helpful to make an explicit description of how to install/include asynquence and the contrib plugins. I tried many things, and my example above is what finally worked. I also think I found one of the examples seemed to imply the following, but I don't think it worked.

    var ASQ = require("asynquence");
    require("asynquence-contrib");
    
  • I think the doc's also imply that asynquence-contrib was within the node_modules/asynquence folder. It is no longer there.

NB I also think I got hung up on a npm version problem. asynquence and/or asynquence-contrib didn't install properly, and upgrading npm from 3.3.12 to 3.7.1 did seem to fix things.

Thanks again!

@getify
Copy link
Owner

getify commented Feb 5, 2016

this is great feedback on improving docs, appreciated.

however, you've raised several substantial/important issues that need to be addressed. I would prefer you file these as separate issues so we can track them:

  1. " .. how to install/include asynquence and the contrib plugins .. I found one of the examples .. but I don't think it worked." This is definitely a problem. It should work that way, and it does in all my usage. We need to figure out why it's not working for you. Please file an issue with exact reproduce details.
  2. "..the doc's also imply that asynquence-contrib was within the node_modules/asynquence folder." I'm not aware of the docs saying that. It was never the case. Can you file an issue with details about where the docs say that so we can clarify/correct?
  3. "..asynquence and/or asynquence-contrib didn't install properly, and upgrading npm from 3.3.12 to 3.7.1 did seem to fix things." This should not be the case currently. There was a period of time several months back where npm3 had come out and changed its rules for how it deals with packages and versions, and asynquence and asynquence-contrib were in fact uninstallable. I released updated versions of both packages that fixed that problem. Also, npm changed their code to stop making some of those breakages. Again, we need to file a separate issue with specific info about how you are installing and what errors are happening. They should not be.

Thanks!

@richb-hanover
Copy link
Author

OK. I will take some time to see what I can re-create and file separate bugs as I nail down test cases. (I was in the throes of starting with new technology, and consequently, not paying full attention to what was happening.) Thanks again for your strong support.

@richb-hanover
Copy link
Author

Responding to the items above: I believe the doc's are correct - my troubles with installation came from misreading the docs. A couple things would make it better: a fully-worked example (see 1. below), and a more clear description of require() (2. below)

  1. I like that there are a lot of examples. But a fully worked example (like the one in the original post above including a comment to "npm install asynquence" and require('asynquence') early in the README would give newbies confidence about getting started.
  2. About item Calling gate with an array of functions #2 above, I was confused about the description of requiring asynquence starting at https://github.com/getify/asynquence#plugin-extensions. To fix that...
  3. I think the documentation would benefit from factoring out the contrib items into contrib/README.md. Also change "Plugin Extensions" heading to "Contributed Plugin Extensions". The intro para should give a link to contrib/README.md, which might have its own description of var ASQ = require('asynquence-contrib'); with an explanation that this will automatically pull in asynquence (This was part of the problem I had thinking that asynquence-contrib was in the same folder as asynquence.)
  4. I don't think I can reproduce the difficulties with installation (sorry.) I have now updated npm, and it's working fine.

TL;DR - I think the tweaks above to the documentation would resolve all these problems (except 4.) Thanks.

@getify
Copy link
Owner

getify commented Feb 10, 2016

Thanks again, fantastic feedback. Will try to digest and make changes as I find some time. :)

@richb-hanover richb-hanover changed the title How do I get data from a wrapped readFile()? Comments on Documentation Feb 10, 2016
@richb-hanover
Copy link
Author

also... s/passsed/passed/g

@legodude17
Copy link

Also look at #93.

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

No branches or pull requests

3 participants