telefonicaid / iotagent-node-lib

Module to enable IoT Agent developers to build custom agents for their devices that can easily connect to NGSI Context Brokers
https://iotagent-node-lib.rtfd.io/
GNU Affero General Public License v3.0
60 stars 85 forks source link

Question: Should this codebase move towards using classes? #970

Open jason-fox opened 3 years ago

jason-fox commented 3 years ago

As I'm sure you know, Node 10 reaches end-of-life at the end of April 2021. Node 12 introduces Public class fields support to Node.

It would therefore make sense to transition to proper class based structure for the codebase.

As an experiment I have converted the Devices directory to use classes and still work with Node 10 and I'm looking on feedback on how to proceed. The full diff can be found here

The new look collects classes with the same interface together (e.g. all files in handlers offer the same interface) and identifies dependencies in a separate folder with a function. It is also easier to see which methods are in the interface and which are just helper methods. The first step shouldn't be creating proper encapsulated constructors - at least whilst node 10 is around, but that could be done later.

Since Node 10 doesn't natively support public class fields there are a few issues I've been having with this pointers e.g:

let mongodb; // single instance
class AlarmDeviceRegistryMongoDB {
    constructor() {
        mongodb = new DeviceRegistryMongoDB();
    }
...
    clear(callback) {
        alarmsInt(constants.MONGO_ALARM, mongodb.clear(callback));
    }
}

works but

class AlarmDeviceRegistryMongoDB {
    constructor() {
        this.mongodb = new DeviceRegistryMongoDB(); // property of the class
    }
...
    clear(callback) {
        alarmsInt(constants.MONGO_ALARM, this.mongodb.clear(callback));
    }
}

runs but doesn't find this in Node 10 and

class AlarmDeviceRegistryMongoDB {
    #mongod; // private
    constructor() {
        this.#mongodb = new DeviceRegistryMongoDB(); // property of the class
    }
...
    clear(callback) {
        alarmsInt(constants.MONGO_ALARM, this.#mongodb.clear(callback));
    }
}

doesn't compile in Node 10.

So how to proceed with this work?

1) Convert the NGSI handlers one by one as a series of separate PRs? (one for device, entities, subscriptions etc.) 2) Create an NGSI handlers big-bang and see if E2E performance is affected? 3) Repeat the exercise with Registries (one for device and a second for group or altogether?) 4) Combine this with the removal of NGSI-v1 maybe?

Basically looking to create a roadmap for this before doing any serious work.

mapedraza commented 3 years ago

Hello Jason, thank for your feedback

It is interesting your comment but at the moment, we think we can not go for it, because there are some production users that does not have NodeJS updated to a V12 version (even if it reaches the end-of-life on April 21) and they still runing Node V10 in their infrastructure. Making iotagent-node-lib incompatible backwards in those customers who are still running V10 Also it means to change the original coding style of the project - which is based on NodeJS modules (CommonJs) - replacing them with public classes

With regards to removing NGSI V1 support (and releasing a new major version of the iotagent-node-lib), we can go for it without being blocked by this topic

jason-fox commented 3 years ago

So the overall timetable goes like this:

1) In the current release 2.14.0 Node 10 is supported 2) In the next release 2.15.0 Node 10 is officially deprecated (released before May 2021) 3) NGSI-v1 can then be removed - without class refactoring 4) Release 3.0.0 drops NGSI-v1 support (and defaults to NGSI-v2) - 5) At some point after April 2021, Node 10 is dropped - the code base still works with it at this point but it is not tested, not officially supported. 6) A subsequent release thereafter i.e. 3.x.0 can then refactor move to using classes since Node 10 will not be supported at that point.

This means we'll be able to use the proper class fields syntax:

class AlarmDeviceRegistryMongoDB {
    #mongod; // private
    constructor() {
        this.#mongodb = new DeviceRegistryMongoDB(); // property of the class
    }
...
    clear(callback) {
        alarmsInt(constants.MONGO_ALARM, this.#mongodb.clear(callback));
    }
}

Refactoring can wait until Node 10 is removed from the GitHubAction test matrix.