3
\$\begingroup\$

I´m currently working on a small microservice application, which will in it´s simplest form just receive a Request from a Formular and and based on containing information perform several other Server-request to different API´s using the Fetch API.

For now I´m just about to create a prototype that contains the Application structure and the promise-chain structure which I´d prefer to use.

I`ve created a prototype which is showing what I´m doing here. My Question is now if this is the right use of promises in which errors i would probably run in a production mode.

I´m currently feeling like I would do it wrong, therefore I´m asking for an advice here.

This is my Controller class which receives the Request:

'use strict';

var indexService = require('../services/index');

class IndexController {
    constructor(router) {
        this.router = router;
        this.registerRoutes();
    }

    registerRoutes() {
        this.router.get('/', this.index.bind(this));
    }

    index(req, res){
        indexService.test()
            .then(data => res.json(data))
            .then(test => console.log("Maybe have to do other things after repsonding to client"))
            .catch(err => console.log(err));
    }
}
module.exports = IndexController;

And this is my Service Class for the business logic:

'use strict';

const fetch = require("node-fetch");
const HttpsProxyAgent = require('https-proxy-agent');

class IndexService {
    constructor() {
    }

    test(){
        var _this = this;
        return new Promise(function(resolve, reject) {
            console.log("INSIDE PROMIES");

           _this.getOne()
            .then(function(data){
               resolve(data);
            })
            .catch(function(error){
                console.log("Not connection to endpoint possible 1");
                reject(error);
            });
        })
         .then(function(data){
             //based on data I call two or three
             console.log("ALSO REACHED THIS ONE");
             if("a"){
                _this.getTwo()
                .then(function(data){
                   return data;
                })
                .catch(function(error){
                    console.log("Not connection to endpoint possible 2");
                    return error;
                });
             }else{
                _this.getThree()
                .then(function(data){
                   return data;
                })
                .catch(function(error){
                    console.log("Not connection to endpoint possible 2");
                    return error;
                });
             }
             return data;
         }).then(function(data){
             console.log(data);
             return data;
         })
         .catch(function(err){
            console.log("ERROR REACHED");
            console.log(err);

            throw "error";
         });
    }

    async getOne(){

        let response = await fetch('https://jsonplaceholder.typicode.com/todos/1', {
            agent: new HttpsProxyAgent("http://xxx:80"),
        });

        if(response.ok){
            console.log("Response OK");
           // console.log(response);
            let data = await response.json();

            return data;
        }else{
            console.log("Response NOT OK");
            //console.log(response);
            return null;
        }


    }

    async getTwo(){

        let response = await fetch('https://jsonplaceholder.typicode.com/todos/1', {
            agent: new HttpsProxyAgent("http:/xxxx:80"),
        });

        if(response.ok){
            console.log("Response OK");
           // console.log(response);
            let data = await response.json();

            return data;
        }else{
            console.log("Response NOT OK");
           // console.log(response);
            return null;
        }
    }

    async getThree(){

        let response = await fetch('https://jsonplaceholder.typicode.com/todos/1', {
            agent: new HttpsProxyAgent("http://xxx:80"),
        });

        if(response.ok){
            console.log("Response OK");
           // console.log(response);
            let data = await response.json();

            return data;
        }else{
            console.log("Response NOT OK");
           // console.log(response);
            return null;
        }
    }

}

module.exports = new IndexService();

To make my question more clear:

  1. Am I using the promises correct here? Should I better do everything in the first then and just chain the async class to use resolve or reject on finish?

  2. What could be a problem with the current design when it gets more complex, because i feel like it is not really readable currently?

  3. Should I use new Promise(function(resolve, reject) instead of async function?

I´m pretty new to this topic and would love to learn from your knowledge!

Thanks.

\$\endgroup\$

1 Answer 1

3
\$\begingroup\$

Promise chain callback hell

The Promise API and now async functions are designed to reduce the number of callbacks (Colloquially known as "callback hell"). You have a total of 16 defined callbacks and by all the console logs I see you are struggling to follow the flow (callback hell)

Your questions

Am I using the promises correct here? Should I better do everything in the first then and just chain the async class to use resolve or reject on finish?

As the chain of data (dependency, content, immediacy, life...) is not clear it is hard to say. At face value. "No" you are not handling the promises correctly, and "Yes" in this case handle the rest in the first resolve in the callback. Let the errors fall through to the calling function.

What could be a problem with the current design when it gets more complex, because i feel like it is not really readable currently?

See the notes below regarding style and readability.

Parallel systems

As for increased complexity using the same approach will quickly become a nightmare. You are creating a node in a parallel data processing system. Parallel processing hates dependencies as they increase the latency of the system and increase the chance of dependency lock.

As it stands you have already set up a high latency system. B Depends on A and will not start until A has responded, thus the best response time is 2 times the response time of the service you are using. Add another dependence and the response is 3 times the service response time. This is an unmanageable condition in the real world.

You may ask what should I do? I can not answer as...

  • I do not know the nature of the data you are fetching,
  • I do not know the nature of the service you are calling, especialy its dependencies on other services (lord forbid if it depended on a response from the server you are running the request on).

The only suggestions is likely impractical, "Keep a local store updated via poling to remove the need for external data dependency chains when responding to requests" and would likely need a redesign of the whole system.

Async functions rule.

Should I use new Promise(function(resolve, reject) instead of async function?

Use async. The rule of thumb is "async means not having to type new Promise" See rewrite

Notes on style and design

  • DON'T!!! throw strings They require special handling and will cause most standard catch handlers to throw an error in the handlers block (the very last place a throw should ever happen). Throw a new instance of an error eg new Error("message") or one of the standard error types eg new RangeError("Value to big")

  • Class syntax is bad. It has forced you to expose what should be private functions getOne, getTwo, getThree. In this case you are creating a single instance object for which the class syntax is completely inappropriate.

  • Use arrow functions to avoid the need for hacks like var _this = this;

  • Use const for variables that do not change.

  • Use var for variables that do change and are function scoped.

  • Use let for variables that are block scoped.

  • Don't create one use variables unless it improves readability. Eg let data = await response.json(); return data; the variable data is just noise. In this case you can return the promise return response.json();

  • Names add abstract meaning. Bad naming results in bugs due to confusion regarding the data being handled. eg you call a promise data in let data = await response.json() The name data is already very generic, but in this case its totally misleading.

  • Avoid the use of null as it is often use as an alias to undefined however it does not have the same semantic meaning, nor does it follow JS convention for undefined return type.

  • If a statement block returns it should not be followed by an else or else if statement block.

  • The two callbacks a promise callback function is passed as arguments should be named appropriately, new Promise((resolve, reject) => would be better as new Promise((fetchOk, fetchFailed) =>

  • Don't create intermediate functions when not required. eg .then(function(data) {resolve(data)}) can be .then(resolve)

  • Exceptions float to the top. You are returning a promise when you call test, It looks like you intend the calling function to include a .catch callback. That means you do not have to handle the catches inside the function test. just let the exceptions fall though to the calling function.

  • Console logging is for debugging only and has no place in release code. You are using console.log to follow flow. Avoid console logging and use dev tools debugger to follow flow.

    One issues with using console is that it forces you to write the code in such a way as to allow for the console expression to exist, often to the detriment of optimal execution and readability.

Rewrite

It is unclear how you intend to handle this code. Nor is the argument router defined. I assume that the function IndexController.index is only for private use. The returned object IndexController is only a husk object to hold the closure. It has no interface.

Note indexService returns a parsed result not the JSON text

I have created only the one module as they are so tightly integrated.

"use strict";
const fetch = require("node-fetch");
const HttpsProxyAgent = require("https-proxy-agent");

const service = {
    URL: "https://jsonplaceholder.typicode.com/todos/",
    get agent() { return new HttpsProxyAgent("http://xxx:80") },
};
async function todos(name){
    const response = await fetch(service.url + name, {agent : service.agent});
    if (response.ok) { return response.json() }
    throw new Error("Fetch of `" + name + "` failed. ");
}    
const index = async () => todos("1").then(todo => todos(todo.foo === "a" ? "2" : "3"));
function IndexController(router) => {
    router.get("/", (req, res) => index()
        .then(todos => /*respond with todos data*/)
        .catch(err => /*respond with service error*/);
    );
}
module.exports = IndexController;
\$\endgroup\$
5
  • \$\begingroup\$ First of all thank you very much for this high quality answer! To make this not too long here in the answer box just one or two things left about the part "On which data the system dependeds". The app is build on top of a cloud platform, The Server instance will get a request from an formular hosted somewhere else. Internally a cloud service will get called and afterwards 3 requests will be made to an API Server outside cloud. \$\endgroup\$ Mar 19, 2019 at 19:58
  • \$\begingroup\$ second question about the code. In the case as describe could i just call the other requests after and like const index = async () => todos("1").then(todo => todos(todo.foo === "a" ? "2" : "3")); of course with other functions than todo. \$\endgroup\$ Mar 19, 2019 at 19:59
  • \$\begingroup\$ @elsololobo I am not sure if I understand the question. You can call index I did not expose it in the example as I was unsure on its use. If you want to call it from outside the module have IndexController return the function directly return index or if you use new IndexController add it to a new object return {indexService: index}; . Renaming it back to the original indexService. \$\endgroup\$
    – Blindman67
    Mar 19, 2019 at 21:14
  • \$\begingroup\$ Okay my Question was a bit unclear. What I mean is that if it would be the correct way to fulfill my requirement of performing several Request in a row. E.g. const index = async () => todos("1").then(todo => todos(todo.foo === "a" ? "2" : "3")); and then afterwards const index2 = async () => todos("3").then(todo => todos(todo.foo === "a" ? "2" : "3")); . \$\endgroup\$ Mar 20, 2019 at 6:29
  • \$\begingroup\$ @elsololobo Without knowing exact nature of the service my advice is tentative and may not be the best solution. If req B is dependent on the results of req A then that is the only way. If the reqs are independent but part of the same res use Promise.all([fetchA(), fetchB()...and so on]).then(.. If the res requires any one of the several reqs use Promise.race See developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/… for more details and links. As you are creating a service be aware the cost is very dependent on all traffic, always do what you can to reduce traffic. \$\endgroup\$
    – Blindman67
    Mar 20, 2019 at 21:48

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.