1

I'm still new to all this asynchronous stuff in JavaScript, and I ran into a bit of a confusing situation when working with some promises. Here's my current code:

exists(filename, targetDir){
    const filepath = path.join(targetDir || this.getTargetDir(), filename);
    // plug in any one method from below
}

When I look into other people's code, I see them resolving values like this (plug into the code above):

// method 1
return request(this.getUrl(filepath))
    .then(res => {
        return Promise.resolve(res.statusCode === 200);
    })
    .catch(() => {
        return Promise.resolve(false);
    });

// method 2
return request(this.getUrl(filepath))
    .then(res => {
        Promise.resolve(res.statusCode === 200);
    })
    .catch(() => {
        Promise.resolve(false);
    });

// method 3
return request(this.getUrl(filepath))
    .then(res => {
        return res.statusCode === 200;
    })
    .catch(() => {
        return false;
    });

// method 4
return new Promise((resolve, reject) => {
    request(this.getUrl(filepath))
        .then(res => {
            resolve(res.statusCode === 200);
        }
        .catch(() => {
            resolve(false);
        };
});

Which ones are correct in this case? Which ones are incorrect? Does it depend on the scenario? Which one of these are recommended? A good explanation would be appreciated, thanks!

Clarification: exists is a class method which returns a Promise which resolves to a boolean, where true means that the URL exists, and false means that it doesn't exist.

Clarification #2: exists should resolve to false if an error occurs.

ifvictr
  • 141
  • 2
  • 3
  • 13
  • @guest271314 Basically I'm trying to find out which way of resolving values from a promise is correct. – ifvictr Aug 13 '17 at 17:45
  • first and third methods are correct and do the same thing – Grundy Aug 13 '17 at 17:49
  • @guest271314, callback passed to `then` **should** return Promise, and if it return not a Promise, then result just would be wrapped. So, in first method just do this manually – Grundy Aug 13 '17 at 17:52
  • @guest271314, yep, if this already not a promise, so, if you return Promise, like in first method, this value would not be wrapped in yet another promise – Grundy Aug 13 '17 at 17:56
  • @guest271314, i mean this two codes return same value in next then – Grundy Aug 13 '17 at 17:58
  • @Grundy No, the two examples are not the same. Method 1 includes `Promise.resolve()` being `return`ed from `.then()`, which is not necessary. – guest271314 Aug 13 '17 at 18:00
  • @guest271314, not necessary, but return _same value_, because if from callback returned not a promise, this construction called somewhere inside – Grundy Aug 13 '17 at 18:01
  • 1
    @guest271314 Yes, `exists` should return a Promise which resolves to `true` or `false`. – ifvictr Aug 13 '17 at 18:28

2 Answers2

4

  • Method 1 is correct but unnecessarily complicated.

  • Method 2 is plain wrong.
    You create new Promises but they ain't populated anywhere so they get disposed when the function ends.
    It is equivalent to:

    return request(this.getUrl(filepath)).catch(err => undefined);

  • Method 3 is the best way to do it.

  • Method 4 would also resolve to the corect value, but it is an antipattern.
    What is the explicit promise construction antipattern and how do I avoid it?

Thomas
  • 11,958
  • 1
  • 14
  • 23
  • what with fourth method? – Grundy Aug 13 '17 at 17:57
  • 2
    @Grundy Updated the Answer (summary: avoid it) – Thomas Aug 13 '17 at 17:59
  • @guest271314 - Method 1 works. It's OK to return a resolved promise from a `.then()` handler. So, presumably that's what he means by "correct". It is not optimal though as there is no reason to return a promise at all, one can just return a plain value from a `.then()` handler as in method 3 which Thomas recommends. – jfriend00 Aug 13 '17 at 18:04
  • @jfriend00 `.then()` returns a `Promise`. Why return `Promise.resolve()`? – guest271314 Aug 13 '17 at 18:08
  • @guest271314 - We're talking about what the `.then()` callback returns, not what `.then()` itself returns. Those are different. A `.then()` callback may return a promise. If it does, then that promise is linked into the existing promise chain. Returning a promise that is already resolved does the same thing as returning just a plain value. Returning a promise that is not yet resolved causes the promise chain to not call the next `.then()` handler until that new promise is resolved (thus inserting itself in the chain). – jfriend00 Aug 13 '17 at 18:27
  • @jfriend00 `Promise.resolve()` resolves the `Promise` immediately, yes? OP does not return `Promise` constructor from `.then()`. In such a case `return false` and `return Promise.resolve(false)` within `.then()` are equivalent, correct? – guest271314 Aug 13 '17 at 18:30
  • 2
    @guest271314 - Yes, they are equivalent and either is allowed. Obviously, `return false` is simpler. – jfriend00 Aug 13 '17 at 18:46
  • @Grundy sry, was akf. *Method 1* is correct as in: *the promise chain will resolve to the correct value*. But it is unnecessarily ugly/complicated as it takes a detour over an Promise wich doesn't add any value whatsoever to the code. And it slightly distracts from what this function does in its essence: resolving to wether `response.statusCode === 200`, or not. – Thomas Aug 13 '17 at 20:26
2

Method 3 is the best.

The next .then() recieves what the current .then() returns. If this return value happens to be a promise, the next .then() is executed after the promise has resolved.

Using method 3, you will be able to append other .then()s to the current chain:

exists('test.txt', '/')
  .then(doesExist => console.log(doesExist ? 'The file exists.' : 'The file doesn\'t exist.'));

This is possible with method 1 and method 4 as well, but method 1 unnecessarily wraps the value in a promise which will resolve immediately.

Method 4 unnecessarily wraps the whole request in a promise. This is called the explicit-construction anti-pattern.

Method 2 does not allow promise chaining. You can't use the value that's wrapped in the Promise.resolve, because both the .then() and the .catch() implicitly return undefined.

PeterMader
  • 6,987
  • 1
  • 21
  • 31
  • Note that `return false` within `.catch()` at Method 3 would result in a resolved `Promise` value at a chained `.then()` - a chained `.catch()` would not be reached – guest271314 Aug 13 '17 at 17:51
  • I guess that's intentional. `exists` should return a promise wrapping `true` if the file exists, and wrapping `false` if the file doesn't exist or if there was an error. `.catch()`s after `exists` should only handle errors that occur in the `.then()`s after exists. – PeterMader Aug 13 '17 at 17:56
  • _"and wrapping false if the file doesn't exist"_ How would `.catch()` be reached? – guest271314 Aug 13 '17 at 17:59
  • Could you please clarify where the particular `.catch()` is? Please see [this](https://gist.github.com/PeterMader/dd54eb1291c513a6af570003a9938abf) gist. – PeterMader Aug 13 '17 at 18:08
  • At Method 3 at code at Question `.catch(() => { return false; });` would result in a resolved `Promise` at chained `.then()` – guest271314 Aug 13 '17 at 18:12