Replacement of the deprecated request library

[bobeal]

Hi,

Starting to work on the Sigfox IoT Agent, I noticed it uses the request library, which is marked as fully deprecated as of Feb 2020 (https://github.com/request/request#deprecated). And so does the IoT Agent node lib.

Has this been spotted? Has a replacement already been identified?

FWIW, there is a list of alternatives here: https://github.com/request/request/issues/3143

[fgalan]

Thanks for the report!

It would be great to change to use a not deprecated one. However, we should try to minimize the impact of changes in the code. Which of the alternatives to requests is the one that provides a closer interface and, at the same time, is mature and widely used? Any thoughts on this?

CC: @jason-fox (as he has been developing in IOTA recently and maybe have some opinion on this)

[jason-fox]

The obvious choice here would be got as the payload most closely follows the old request library.

I also had a look at axios and it uses data not json in the payload which is annoying and would need many more changes.

request is used in around 15 modules, and all of the callbacks and all the response handlers would need refactoring to some extent.

got uses the Promise API, so this would be an opportunity to start removing the callback hell introduced by the async Promise API mixin (which is totally unnecessary now we're Node 10). Moving to async ... await may be a step too far, but Promise chains rather than embedded callbacks would make sense.

As an experiment, I've looked at swapping out request in two modules: iotManagerService.js and entities-NGSI-LD.js

https://github.com/telefonicaid/iotagent-node-lib/compare/feature/842_ngsi_ld...jason-fox:feature/axios?expand=1

You can see the result here - it isn't as difficult as it looks, as it is mainly moving the existing code around - also nock needs an upgrade to run the tests with this.

iotManagerService.js uses a promise chain, but most request calls just need success and error handlers.

[TrueSkrillor]

@jason-fox I don't get your argument why moving to async ... await would be too far. The ECMAScript 2017 async functions are supported since Node version 7.6 and also available in all major browsers for over two years. If we are going for the major Promise rework of this library, using async ... await from the beginning simplifies asynchronous functions and makes them easier to code and understand. We should also consider exposing a Promise-based API rather than the one based on callbacks. I've seen other libraries performing this change in the past and they determined whether to use Promise or callback by the type of the callback provided (typeof callback === 'undefined' => Promise; typeof callback === 'function' => callback). This way there are no breaking changes in the API while still offering a Promise-based API.

[jason-fox]

If we are going for the major Promise rework of this library

I have no disagreement here - personally I'd prefer a move towards Promises over callbacks, but that would be a significant change in the programming style of the library. Any such change in programming would have to be approved by the owners of this repo before the work would commence.

Also from past experience it is difficult to get broad wide-reaching changes approved in a timely manner if other significant changes are going on at the same time, so it may be the case that a window for making changes needs to be identified or a strategy created for making the changes incrementally.

[jason-fox]

I've been looking at got as a replacement for therequest library. The result of the investigation can be found here: https://github.com/telefonicaid/iotagent-node-lib/compare/master...jason-fox:feature/got?expand=1

Quick and Dirty

For a minimal change, it looks like a simple shim will suffice, for example in securityServiceKeystone.js, the following change is sufficient to remove the request dependency :

-const request = require('request');
+const got = require('got');
 const errors = require('../../errors');
 const config = require('../../commonConfig');
 const intoTrans = require('../common/domain').intoTrans;
 const logger = require('logops');
 const context = {
     op: 'IoTAgentNGSI.SecurityService'
 };

+function request(options, callback) {
+    got(options.url, {
+        method: options.method,
+        json: options.json,
+        headers: options.headers,
+        throwHttpErrors: false,
+        retry: 0
+    })
+        .then((response) => {
+            return callback(null, response);
+        })
+        .catch((error) => {
+            return callback(error);
+        });
+}

This would be a simple fix and could be replicated across all classes using request - the problem is that request is used all over the place and you don't really want to keep const request = require ('request-shim'); all the time. However using a common shim placed in /test/tools would probably be the best way to eliminate request from the unit tests.

Eliminate a callback

A better approach for the main codebase would be to eliminate at least one layer of callbacks - I've tested and example of this with devices-NGSI-LD.js - the code just moves the existing error handlers into the calling

Use the promise chain

I think the best approach would be to keep the existing handlers but place them in the got promise chain - something like this from entities-NGSI-LD.js:

     logger.debug(context, 'Querying values of the device in the Context Broker at [%s]', options.url);
     logger.debug(context, 'Using the following request:\n\n%s\n\n', JSON.stringify(options, null, 4));

-    request(
-        options,
-        generateNGSILDOperationHandler('query', entityName, typeInformation, token, options, function (error, result) {
-            if (error) {
-                callback(error);
-            } else {
-                NGSIUtils.applyMiddlewares(NGSIUtils.queryMiddleware, result, typeInformation, callback);
-            }
+    got.get(options.url, {
+        headers: options.headers,
+        throwHttpErrors: false,
+        retry: 0
     })
-    );
+        .then((response) => {
+            return queryEntityHandlerNgsiLD(response, typeInformation, options.headers, entityName, token, callback);
+        })
+        .catch((error) => {
+            logger.error(context, 'Error found executing query action in Context Broker: %s', error);
+            alarms.raise(constants.ORION_ALARM, error);
+            return callback(error);
+        });
 } 

So the question to the Telefonica team is how do you want to proceed with this?

[jason-fox]

Obviously it makes sense to wait for #995 and not modernize anything NGSI-v1 related at all. As a start, adding a PR to use a shim for the NGSI-v2 and NGSI-LD unit tests could be a simple broad change across many files and reduce the scope of the problem. But what about the main code base? Shim first and update to promisify the code later maybe?

[fgalan]

Fixed in PR https://github.com/telefonicaid/iotagent-node-lib/pull/1119 and many follow up ones (see details in that PR conversation, specially this comment and this other)