5

I am thinking could it be a good approach to always return promises in functions in JavaScript?

Let's imagine the case where we have a function that validates a username. A master function simply utilises 2 other functions that perform different checks.

Please note, all functions names are just examples.

// Returns a boolean
function validateUsername (username) {
  return validateUsernameFormat(username) &&
    isUsernameReserved(username);
}

// Returns a boolean
function validateUsernameFormat (username) {
  return typeOf(username) === 'string' &&
    username.match(/^\[a-z0-9]{8,20}$/);
}

// Returns a boolean
function isUsernameNotReserved (username) {
  return ['igor', 'kristina'].indexOf(username) === -1;
}

Now let's imagine we augment our validation in the future by calling API to check if a given username already exists in our database.

// Now returns a promise
function isUsernameNotReserved (username) {
  return API.checkIfUserNameAlreadyExists(username);
}

This would mean we will also now have to change the master validateUsername function since it now also needs to return promise. This would also probably mean we will have to modify all functions that use validateUsername function.

But what If we had all function in promises from scratch?

Option A - All functions return promises

// Returns a promise
function validateUsername (username) {
  return validateUsernameFormat(username)
    .then(() => {
      return isUsernameReserved(username);
    });
}

// Returns a promise
function validateUsernameFormat (username) {
  return (
    typeOf(username) === 'string' && username.match(/^\[a-z0-9]{8,20}$/) ?
    Promise.resolve() : Promise.reject()
  );
}

// Returns a promise
function isUsernameNotReserved (username) {
  return (
    ['igor', 'kristina'].indexOf(username) === -1 ?
    Promise.resolve() : Promise.reject()
  );
}

Now if we want to augment isUsernameNotReserved with asynchronous API call, we don't need to change anything else.

Option B - Only functions calling another functions return promises

Also, another option would be write functions in promises that call another functions. In that case, only validateUsername should be written as a promise from scratch.

Is this a good approach? What could be the drawbacks apart from performance?


Upd: ran a simple performance test and though running consequent promises is slower, it practically should not make any difference since running 100000 consequent functions takes ~200ms, while running 1000 takes ~3ms in Chrome. Fiddle here https://jsfiddle.net/igorpavlov/o7nb71np/2/

igorpavlov
  • 3,576
  • 6
  • 29
  • 56
  • 1
    No. First, only functions that do async operations would potentially benefit and then out of those, you restrict what else the function could return. This not something to just do arbitrarily. – Scott Marcus Dec 07 '17 at 16:20
  • Downvote? Seriously? This can dramatically decrease the development time during continuous integration. As per returning restrictions you can always resolve with different values. – igorpavlov Dec 07 '17 at 16:23
  • 1
    The down vote is because this is not an appropriate question for Stack Overflow. – Scott Marcus Dec 07 '17 at 16:24
  • I expected people to attempt to answer this question "Is this a good approach? What could be the drawbacks apart from performance?" I didn't ask for any personal opinions. – igorpavlov Dec 07 '17 at 16:26
  • Stack overflow isn't for these questions. This should be asked in CodeReview https://codereview.stackexchange.com/ – Eli Richardson Dec 07 '17 at 16:30
  • In order to answer if it is a good idea, one must consider their own knowledge, experience and possible use cases. Since we all have different backgrounds, the only answers you could get would be opinion based. – Scott Marcus Dec 07 '17 at 16:47
  • Try to [avoid asynchronous functions when they are not necessary](https://stackoverflow.com/a/45448272/1048572). – Bergi Dec 07 '17 at 18:03
  • @EliRichardson No. Codereview is for judging production-ready code, not about hypothetical example snippets. This question might fit on [softwareengineering.SE], but it's on-topic here as well. – Bergi Dec 07 '17 at 18:05
  • @Bergi Hmm, my mistake then. I was under the impression Stack Overflow wasn't supposed to be used for questions like this. – Eli Richardson Dec 07 '17 at 18:08
  • 2
    @EliRichardson I would think it matches the description of ["*a practical, answerable problem that is unique to software development*"](https://stackoverflow.com/help/on-topic). It doesn't need to be a problem of some specific code, rather open questions about generic problems are fine as well unless their answer is completely opinion-based (as Scott Marcus mentioned). Looking at jfriend's excellent answer, this question here is quite answerable though. – Bergi Dec 07 '17 at 18:13

1 Answers1

9

Should I always return promises in all functions in JavaScript?

No.

If you have a function that performs an asynchronous operation or may perform an asynchronous operation, then it is reasonable and generally good design to return a promise from that function.

But, if your function is entirely synchronous and no reasonable amount of forethought thinks this will sometime soon contain an asynchronous operation, then there are a bunch of reason why you should not return a promise from it:

  1. Asynchronous code writing and testing is more complicated than synchronous code writing and testing. So, you really don't want to make code harder to write and test than it needs to be. If your code can be synchronous (and just return a normal value), then you should do so.

  2. Every .then() handler gets called on the next tick (guaranteed asynchronously) so if you take a whole series of synchronous operations and force each function to wait until the next tick of the event loop, you're slowing down code execution. In addition, you're adding to the work of the garbage collector for every single function call (since there's now a promise object associated with every single function call).

  3. Losing the ability to return a normal value from a synchronous function is a huge step backwards in the language tools that you can conveniently use to write normal code. You really don't want to give that up on every single function.

Now if we want to augment isUsernameNotReserved with asynchronous API call, we don't need to change anything else.

A good API design would anticipate whether an asynchronous API is relevant or likely useful in the near future and make the API async only in that case. Because asynchronous APIs are more work to write, use and test, you don't want to unnecessarily just make everything async "just in case". But, it would be wise to anticipate if you are likely to want to use an async operation in your API in the future or not. You just don't want to go overboard here. Remember, APIs can be added to or extended in the future to support new async things that come along so you don't have to over complicate things in an effort to pre-anticipate everything that ever might happen in the future. You want to draw a balance.

"Balance" seems like the right word here. You want to balance the likely future needs of your API with developer simplicity. Making everything return a promise does not draw a proper balance, as it chooses infinite flexibility in the future while over complicating things that don't need to be as complicated.


In looking at a couple of your particular examples:

validateUsernameFormat() does not seem likely to ever need to be asynchronous so I see no reason for it to return a promise.

isUsernameNotReserved() does seem like it may need to be asynchronous at some point since you may need to look some data up in a database in order to determine if the user name is available.

Though, I might point out that checking if a user name is available before trying to just create it can create a race condition. This type of check may still be used as UI feedback, but when actually creating the name, you generally need to create it in an atomic way such that two requests looking for the same name can't collide. Usually this is done by creating the name in the database with settings that will cause it to succeed if the name does not already exist, but fail if it does. Then, the burden is on the database (where it belongs) to handle concurrency issues for two requests both trying to create the same user name.

jfriend00
  • 683,504
  • 96
  • 985
  • 979
  • Regarding slowdown and garbage collection, this should have rather marginal impact unless ran in a really tight loop. The "next tick" happens immediately, there is no extra delay unless from the context change. Short-lived objects like promises do not put much pressure on GC, and in case of `async`/`await` I would expect a compiler to even completely optimise them away. – Bergi Dec 07 '17 at 18:02
  • @Bergi - The OP is talking about designing EVERY function to return a promise. Do you not think that will have any impact on performance? To quantify, it would require deciding what kinds of tests would give you a good answer and then running in several environments. – jfriend00 Dec 07 '17 at 18:22
  • I don't think it doesn't have *any* impact, I think that the impact will not be noticeable for most functionality. The other two reasons, especially the correctness one (async code being easier to mess up and harder to reason about because of concurrency), are much more important than the performance argument. – Bergi Dec 07 '17 at 21:06
  • @Bergi - OK, I'll agree with that. I tried to make a performance test on jsperf, but their async test handling is busted. Ran out of energy to code the test manually. – jfriend00 Dec 07 '17 at 21:07
  • Also a jsperf benchmark would be exactly that "tight loop" I was talking about, where the difference between a call to an empty function and a call to an empty async function is huge. I meant that the user won't perceive the difference in most cases, say an event handler that calls a *few* other functions. – Bergi Dec 07 '17 at 21:17
  • @Bergi - Yeah, but the OP isn't talking about the difference between making one function return a promise vs. not. They're talking about making it something they do with ALL their functions. That will add up. But, agree that the programming complexity reasons are what kill it as an idea to start with. Also adds new opportunities for race conditions. – jfriend00 Dec 07 '17 at 21:18