0

EDIT: the conclusion reviewers came to about this being a duplicate is wrong and stems from misunderstanding of the actual question. Please see the EDIT at the bottom of the post for a summary and resolution, as well as the comments to this question confirming that this is not in fact a duplicate.

ORIGINAL QUESTION: I'm handling the logic for backend communication for our team's frontend. All requests follow this pattern:

  1. take input (varies)
  2. preprocess the input (consistent for all inputs)
  3. send input to the backend (varies, has developer-defined conditional logic)
  4. postprocess backend response (consistent for all)
  5. pass response to the rest of the frontend

We recently stumbled into a few calls that were failing silently, upon investigation we realized the issue was that the way the logic was originally written (via promises) required an individual .catch() call, which we missed in a few places.

While looking at this module I also noticed that it's accumulating a lot of boilerplate, mainly every single call to the backend is written as:

function performOperation(input) {
  input = preprocess(input); // step 2
  return new Promise((resolve, reject) => {
    if (env.local) {
      resolve(/* logic to pull from local cache */);
    } else {
      thirdPartyFetchCall(input).then((response) => {
        resolve(response);
      }).catch((error) => {
        reject(error);
      });
    }
  }).then((output) => {
    return postprocess(output); // step 4
  }).catch((error) => {
    // this is for catching postprocess errors
    console.error(error);
  });
}

There are several dozen calls following this pattern and I wanted to clean it up a bit, as well as making the final error dump automatic (preprocess benefits from being in the same thread, so its throws are automatically logged, that's not the case with postprocess).

My idea was wrapping it in a promise factory that the developer could then call as follows:

function performOperation(input) {
  return factory(input).send((processedInput) => {
    if (env.local) {
      return (/* logic to pull from local cache */);
    } else {
      return thirdPartyFetchCall(processedInput);
    }
  });
}

The promise factory would take care of preprocess and postprocess calls, as well as dumping the postprocess error. Problem is that I'm not sure how to tell from the factory if thirdPartyFetchCall will result in resolve or reject while keeping the logic independent. I could call .then and .catch on the return of the function being passed to .send but in the case of env.local, it will return a non-promise value and convincing other developers that promisifying it manually is less error-prone than remembering to add the .catch at the end of the chain is unlikely. How do you recommend I handle it safely from the factory, just test if the object is a promise? If I simply call resolve on it from within the factory, will the .catch clause still fire in response to an error thrown by thirdPartyFetchCall without an explicit reject call in the Promise declaration? Here is an example of the factory code to go along with the above:

function factory(input) {
  return new Promise((resolve, reject) => {
    return { send: (request) => {
      resolve(request(preprocess(input)));
    }
  }).then((output) => {
    return postprocess(output);
  }).catch((error) => {
    console.error(error);
  });
}

Note that in above factory reject never gets explicitly called in the promise declaration, but I still want the catch at the end of the chain to execute in response to an error thrown within postprocess call or a reject call within thirdPartyFetchCall promise returned by the request function.

EDIT: Ok, I feel stupid now. The whole factory idea led me astray. I got stuck in implementation details when really my main question was whether .catch() clause would still fire on a promise where reject has not been explicitly called within the declaration. And the answer is yes, it will - which I was able to test by simplifying the test further and running it in node:

function test() { throw 'error'; }
let p = new Promise((resolve, reject) => {
  resolve(test());
}).then((r) => {
  console.log('success');
}).catch((r) => {
  console.log('error');
});
Alexander Tsepkov
  • 3,946
  • 3
  • 35
  • 59
  • Don't wrap the promise, return it. – Patrick Roberts Sep 20 '18 at 04:59
  • I don't understand why you write `return { send: request => ... }` – [the return value of the executor lambda is ignored](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise#Syntax). – Mulan Sep 20 '18 at 04:59
  • One thing about promises worth mentioning is that even if you return a non-promise value from a promise context, you can still chain it with `.then`. Take that into consideration and you may significantly simplify your solution. – woozyking Sep 20 '18 at 05:00
  • 1
    This question seems to be about how to write a program with swappable storage modules – probably too broad for SO in this format. – Mulan Sep 20 '18 at 05:01
  • Well, the idea is to be able to switch between cache and backend, not handle other storage modules, but yes, the logic would be similar. – Alexander Tsepkov Sep 20 '18 at 05:03
  • You might want a higher order function over `performOperation` like `addPrePostProcess(performOperation)(input)` to remove the need to pre and post in every function. or move it up a layer at the entry point to these functions. the function is doing a lot. – Andy Ray Sep 20 '18 at 05:03
  • @PatrickRoberts the difference from that question is that I can't guarantee that the function called from within the main promise will in fact return a promise, if I was calling `thirdPartyFetchCall`, then yes, that simplification would work. – Alexander Tsepkov Sep 20 '18 at 05:07
  • @AndyRay that's exactly what I'm trying to do with the `factory`. It is nothing more than a higher order function for preprocess, postprocess, and error catching. – Alexander Tsepkov Sep 20 '18 at 05:09
  • This is not a duplicate of that question, even if the common wisdom from that Q&A applies here. – Mulan Sep 20 '18 at 05:09
  • 1
    @AlexanderTsepkov `(env.local ? Promise.resolve(...) : thirdPartyFetchCall(...)).then(...)` No need for the `Promise` constructor. – Patrick Roberts Sep 20 '18 at 05:15
  • @user633183 my specific question is if the above factory code would be able to handle the error propagation that the original logic is designed to handle without explicit use of `reject` call in the declaration (which I'm unable to do because I can't decide if the call returns a promise), basically I'm doing the equivalent of the suggested answer in https://stackoverflow.com/questions/27746304/how-do-i-tell-if-an-object-is-a-promise. I'm not asking the more broad question about the design. – Alexander Tsepkov Sep 20 '18 at 05:17
  • Other suggestion is the make `performOperation` an `async` function, which will implicitly wrap any returned non-promise value in `Promise.resolve()`. – Patrick Roberts Sep 20 '18 at 05:17
  • @PatrickRoberts the Promise.resolve() solution would work, but this portion would not be in 1-time written factory code but code we write for every call, hence it would be susceptible to be forgotten the same way we've forgotten the catch clause a few times (that's the part I mentioned about trying to convince other developers that this solution is in fact cleaner). However, I may be overthinking it, since it's easy to visually verify that return types would be inconsistent if the resolve clause is forgotten. – Alexander Tsepkov Sep 20 '18 at 05:22
  • I like the idea of a promise factory but I think `factory` should be a little more generic and it should only return a `promise` and not an object that contains the next method to call. That way this pattern could scale through out your application. – trk Sep 20 '18 at 05:28
  • @AlexanderTsepkov use TypeScript. – Patrick Roberts Sep 20 '18 at 05:28
  • If the function might return a promise might or might not, just do `Promise.resolve( call() ).then(...)` that way you got unified and clean behaviour – Jonas Wilms Sep 20 '18 at 05:31

0 Answers0