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

Making it clearer? #26

Open
greggman opened this issue Mar 23, 2015 · 7 comments
Open

Making it clearer? #26

greggman opened this issue Mar 23, 2015 · 7 comments

Comments

@greggman
Copy link
Contributor

Reading through this I couldn't help wonder if it's not that clear. To someone not familiar with promises the sync code is super clear both in how it's implemented and how it's used

impl

  function readJSONSync(filename) {
    return JSON.parse(fs.readFileSync(filename, 'utf8'));
  }

usage

 var obj = readJSONSync('somefile.json');
 // do something with obj

If you're familiar with callbacks the callback version is also clear

impl

  function readJSON(filename, callback){
    fs.readFile(filename, 'utf8', function (err, res){
      if (err) return callback(err);
      callback(null, JSON.parse(res));
    });
  }

usage

readJSON(filename, function(err, obj) {
   if (err)  { throw; } // or whatever
    // do something with obj
});

But even though it's explained indirectly it seems like it's not clear to a noob how to use the promise version.

impl

  function readJSON(filename){
    return readFile(filename, 'utf8').then(JSON.parse);
  }

usage

???

Would it be good to add a usage example?

readJSON("somefile.json").then(function(obj) {
   // do something with obj
});

Or possibly

readJSON("somefile.json").then(function(obj) {
   // do something with obj
}).catch(function(err) {
   // do something about error
});

One difference from the first promise example (the one without catch) vs the original sync example, the sync example will throw if reading or parsing fails. The promise example will silently fail.

@ForbesLindesay
Copy link
Owner

Thanks for the feedback. It's great to have as many perspectives as possible.

The simplest usage of the promise version is:

readJSON('somefile.json').done(function (result) {
  // do something with obj
}, function (err) {
  // do something with err or delete this function
});

On the other hand, what you suggest is equally fine if you want to do something with the result. The thing is that promises are flexible, so it's difficult to really give one canonical usage.

As for the callback code being easy to understand. I agree, the code you post for a callback example looks easy to understand, but it's deceptive. The code you posted has a bug and behaves very differently to the synchronous code when it comes to error conditions. The simplest possible way of doing this correctly looks like:

function readJSON(filename, callback){
  fs.readFile(filename, 'utf8', function (err, res){
    if (err) return callback(err);
    try {
      res = JSON.parse(res);
    } catch (ex) {
      return callback(ex);
    }
    callback(null, res);
  });
}

which I don't think is particularly easy to follow :)

@greggman
Copy link
Contributor Author

It sounds like you missed my point entirely. Maybe I suck at making my point.

I totally agree promises are awesome and handle things way better than callbacks.

My point is it's clear how to use the sync version, it's not clear, from this document, how to use promises. Not because promises are hard, they aren't. Because there's no actual example of usage in this document.

The usage of the sync version is clear

var obj = readJSONSync('somefile.json');
// do something with obj

You don't have to tell anyone how to use it. They know because they are programmers and sync is the norm. Similarly, if they've done any javascript they probably know callbacks. Many web APIs and many node APIs use callbacks

But if they're looking for how to use promises they likely need an example. All this document provides is examples of implementing readJSON using promises. It does not provide an example of using that promised based implementation of readJSON

It seems like the document would be much clearer if you added an example of actually using the promise version of readJSON. On top of that it would best to show how to handle errors because unlike the sync version where an error throws, not handling the error in the promise version is a silent failure.

@ambujpunn
Copy link

I agree with @greggman. @ForbesLindesay Promises are awesome, but as a noob to promises and newbie to javascript, I would love to see some examples to see how we can use Promises more in action. I would really appreciate if you could add some.

@brodybits
Copy link

A suggestion on my part would be to add a very simple usage sample to the web site then link to a tutorial such as HTML Rocks. (https://github.com/jakearchibald/es6-promise links to http://www.html5rocks.com/en/tutorials/es6/promises/)

@greggman
Copy link
Contributor Author

I saw a great talk by a harvard professor. Basically he said after teaching for many years and with lots of data to prove his beliefs were wrong he realized he had forgotten what it was like to not understand something.

In other words he's explaining something to his students and in his mind is 100% obvious but that's because he has years of that knowledge in his head. I can't remember what it was like to not understand it.

Copying again from my original post. Every JavaScript programmer knows how to use the sync version

var obj = readJsonSync(filename);
console.log(obj);

Probably every JavaScript programmer know show to use a callback

readJson(filename, function(err, obj) {
   console.log(obj);
});

People not familiar with promises have no clue how to use promises. This document doesn't tell them how to do it. It only tells them what they fix and how to reimplement a function to use them but it doesn't say how to use those reimplemented functions.

To be complete (or at least more complete) it needs an example like this

readJson(filename).then(function(obj) {
   console.log(obj);
});

That simple example of actually using the result is not in the document.

@ForbesLindesay
Copy link
Owner

Since you now do understand it, and presumably didn't at some point not very long ago, would you consider submitting pull requests to update the docs in https://github.com/ForbesLindesay/promisejs.org/tree/master/translations/en/pages

It would be a great help.

@meeple142
Copy link

meeple142 commented Jun 18, 2016

Thanks @greggman was having this exact problem. Because of this code

  function readJSON(filename){
    return readFile(filename, 'utf8').then(JSON.parse);
  }

I thought that when a promise resolves it could be a value, or a promise??? --- but that also didn't totally make sense to me. After playing with it for a while I actually guessed how to use it. I came up with this

readJSON('cool.json').then(function (guts) {
   console.log(guts);
});

Almost exactly what you suggested. Now (along with your other suggestions) I get how to actually use it.
Thank you for confirming!

@ForbesLindesay
It's this site that has taught me what promises are and their benefit. I really like your example from sync to async to promises. I just didn't know how I was supposed to use the neat readJSON function.
So I totally second adding a usage example at the end.

Thank you both!

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

5 participants