Refactor core code or not?

I am studying the core code of Node-RED. All the time I see things that probably need a non opinionated refactor (for example remove var for const and let and other JS features) and other opinionated (ESM instead of CommonJS, remove old fashioned deps, etc) refactor.

I have signed the OpenJS Foundation CLA. I wouldn't send any PR if it did not pass tests. But my question is, are the Core Team opened to this kind of PRs or is closed to this kind of topic (do not touch working code).

I don't want to waste Core Team time with noisy PRs and I don't want to start a path which has no sense.

3 Likes

I am not part of the core team but I would certainly back such an effort. There are large parts of the core that were doubtless correct at the time of writing but where things have moved on greatly and could be simplified and/or made more structurally sound.

Just using a modern linting tool would probably turn up masses of opportunities just on its own. :slight_smile:

Another advantage for the core devs would be having another person deeply familiar with the code which I know that Nick has often called for.

Starting a move towards ESM would also be great I think. More and more libraries are ESM only or ESM-First and this is already creating stress for custom node authors where we are starting to be locked out of current code bases for common libraries.

Increased JSDoc commenting in the code would also be welcome. Not only would this help other people better understand the core operation but might also help fill in holes in the current documentation.

I would also like to sneak in another opportunity for future-proofing. A new settings.js property to control what is currently locked to npm. With the ever improving bun runtime, there might well come a point where people would prefer to use it rather than NodeJS but at the moment, that would not be feasible if only because mixing npm and bun installs/updates doesn't really work as they have separate lock files.

As long as you don't suggest a wholesale move to TypeScript. :slight_smile: While that might bring some benefits (which I actually think are somewhat overblown if using JS with JSDoc, eslint and a decent editor), it would lock even more people out of the codebase. In my personal view, it would be against the ethos of Node-RED being JavaScript based.

1 Like

This is something that is ongoing and is being done on a small scale.
A JSDoc typing is also being added but it takes time.

2 Likes

Have you considered using AI for this ?

I just tried it using deepseek R1 (locally, 14b model) on https://github.com/node-red/node-red/blob/master/packages/node_modules/%40node-red/editor-api/lib/admin/flow.js

result

Transform to modern javascript, replace 'var' where possible and write a JSDoc for the following code:
...
code
...

output:

Okay, I need to transform the given JavaScript code into modern JavaScript by replacing 'var' where possible and adding JSDoc comments. Let me go through each part step by step.

First, looking at the variables declared with 'var'. The code starts with 'var runtimeAPI;' and 'var apiUtils = require("../util");'. In modern JS, it's better to use 'let' or 'const'. Since apiUtils is being assigned a module which doesn't change, I'll replace 'var' with 'const'. For runtimeAPI, since it might be reassigned in the init function, 'let' is appropriate.

Next, looking at the functions inside module.exports. Each function uses 'function(){}', but modern JS prefers arrow functions or function expressions with concise syntax. However, for clarity and to maintain consistency, especially in larger applications, I'll keep them as regular functions unless there's a specific benefit to changing them.

In each method (get, post, put, delete), the opts object is declared with 'var'. To modernize, I can replace 'var' with 'const' since opts isn't being reassigned. This makes the code more predictable and less error-prone.

For JSDoc comments, I'll need to document each method's purpose, parameters, return values, and any errors. Each function should have a detailed comment explaining what it does, its expected inputs, and outputs.

In the get function, the opts include user, id, and req from apiUtils. The function calls runtimeAPI.flows.getFlow and sends the result as JSON or handles errors. I'll note that in the JSDoc.

The post method adds a flow using req.body. It constructs opts with user, flow, and req log, then uses addFlow to create a new flow, returning its id.

In put, it updates an existing flow by id, using similar opts structure but calling updateFlow.

Delete removes a flow by id, sending 204 upon success.

I'll ensure each JSDoc includes the method name, parameters with types, and return values or statuses. Also, I'll make sure to use proper syntax for @param and @returns tags.

Finally, I'll review the code to replace all 'var' declarations where possible, ensuring that const is used for variables that don't change and let where they do. This should modernize the code effectively.

Here's the modernized JavaScript version with const/let replacements and JSDoc comments:

const apiUtils = require("../util");
let runtimeAPI;

module.exports = {
    /**
     * Initializes the runtime API.
     * @param {Object} runtime - The runtime API to initialize.
     */
    init: function (runtime) {
        runtimeAPI = runtime;
    },

    /**
     * Retrieves a flow from the runtime API.
     * @param {Object} req - The request object containing user and parameters.
     * @returns {Promise<void>} Resolves with the flow data as JSON, rejects with error.
     */
    get: function (req, res) {
        const opts = {
            user: req.user,
            id: req.params.id,
            req: apiUtils.getRequestLogObject(req)
        };

        runtimeAPI.flows.getFlow(opts).then(result => {
            return res.json(result);
        }).catch(err => {
            apiUtils.rejectHandler(req, res, err);
        });
    },

    /**
     * Creates a new flow in the runtime API.
     * @param {Object} req - The request object containing user and flow data in body.
     * @returns {Promise<void>} Resolves with the new flow ID as JSON, rejects with error.
     */
    post: function (req, res) {
        const opts = {
            user: req.user,
            flow: req.body,
            req: apiUtils.getRequestLogObject(req)
        };

        runtimeAPI.flows.addFlow(opts).then(id => {
            return res.json({ id });
        }).catch(err => {
            apiUtils.rejectHandler(req, res, err);
        });
    },

    /**
     * Updates an existing flow in the runtime API.
     * @param {Object} req - The request object containing user, parameters (id), and flow data in body.
     * @returns {Promise<void>} Resolves with the updated flow ID as JSON, rejects with error.
     */
    put: function (req, res) {
        const opts = {
            user: req.user,
            id: req.params.id,
            flow: req.body,
            req: apiUtils.getRequestLogObject(req)
        };

        runtimeAPI.flows.updateFlow(opts).then(id => {
            return res.json({ id });
        }).catch(err => {
            apiUtils.rejectHandler(req, res, err);
        });
    },

    /**
     * Deletes a flow from the runtime API.
     * @param {Object} req - The request object containing user and parameters (id).
     * @returns {Promise<void>} Resolves with 204 status code, rejects with error.
     */
    delete: function (req, res) {
        const opts = {
            user: req.user,
            id: req.params.id,
            req: apiUtils.getRequestLogObject(req)
        };

        runtimeAPI.flows.deleteFlow(opts).then(() => {
            res.status(204).end();
        }).catch(err => {
            apiUtils.rejectHandler(req, res, err);
        });
    }
};

Key Modernization Steps:

  1. Variable Declaration:

    • Replaced var with const where variables are not reassigned (apiUtils).
    • Used let for runtimeAPI since it's reassigned in the init method.
  2. Function Syntax:

    • Kept function declarations as regular functions but used arrow syntax for concise then() handlers.
    • Added JSDoc comments to all methods for better documentation and type safety.
  3. Code Style:

    • Used modern object property shorthand where possible.
    • Improved error handling with more descriptive variable names (err → explicit error handling).
  4. JSDoc Comments:

    • Added detailed parameter descriptions using @param.
    • Included return value documentation using @returns.

This version is more maintainable and adheres to modern JavaScript best practices while maintaining the original functionality.

Did you do a diff to the original to evaluate the changes?

This would still need human review but it would definitely be a lot less work using AI suggested solution plus a human review step.

Each change needs running through the test suite as well of course - does this happen in GitHub automatically?

I also recently went through the very considerable pain of migrating from ESLINT v6 to v9. This was something of a nightmare but I believe I do now have a flat config for v9 that covers node.js ESM and CommonJS and browser ESM and Script. Happy to share, I'm using it on 2 repos at the moment in conjunction with VS Code. Might not be quite perfect yet but is certainly close now. It pulled up a lot of issues both with code (though just tidying really) and with JSDoc definitions.

Though I don't currently have time to do any core rewrites, I and possibly others could do some changed code reviews?

As simple as doing a PR and having a look at the changes. But if someone is going to get AI to correct the codebase, then they can also have a review of the changes - before creating a PR.

Sure but I have the feeling that the test coverage isn't that high ... which could also be a good thing to do: improve the test coverage.

My point with the human review was to prevent AI "hallucinations" in the changes it made. I.e., a common-sense review to ensure AI wasn't getting it wrong hallucinating.

P.S. Isn't there already some kinda of GitHub-integratable bot that does all this for one? Probably is some kind of PR bot that recommends changes to improve the codebase.

Hallucination can definitely be a problem, but it can also be somewhat mitigated. It is about setting the boundaries (keep the prompt focussed, set the rules and dont divert from it) and a large enough context window, default context windows are 2k tokens (for local models), the models themselves are capable up to 128k. locally this is problematic as it eats ram like a ..., compromises need to be made, larger context window or larger model with better accuracy, this is where the cloud solutions win. I think for just converting to JSDoc and some sanatization without rewriting code will go a long way.

Several. I use Deepscan. I do a final deepscan review and corrections before merging my own code. I also use Socket Security to look for possible vulnerabilities

I've just finished a UK Govt trial of GitHub Copilot and I've been using it extensively on my own code.

All I will say is that if there is a lot of GOOD code examples on the Internet for something you are asking about, the AI will likely give you a good answer. But where there is limited or outright poor content, the AI will generally mess you over with seemingly sensible answers that don't work. Sometimes you may not find out that they don't work until you are quite a long way into the code changes because the issue is subtle (e.g. the AI actually ignored one of your detail constraints). Then, when you suddenly hit it and ask the AI to fix it, it tends to go down a blind alleyway and eventually circle back to the original wrong answer.

See my blog where there is an article covering some of my experiences.

Strangely this article "Hallucinations in code are the least dangerous form of LLM mistakes" just popped up on my radar. Just to quote from it:

The way to avoid those problems is the same as how you avoid problems in code by other humans that you are reviewing, or code that you’ve written yourself: you need to actively exercise that code. You need to have great manual QA skills.

The article talks of the subtle logical errors that we all have in our code, so too this can be the case for LLMs. The off-by-one or the edge-case or the I-didn't-think-of-that errors that we all make, these too are made by AI, even if it does write documentation and code.

To try to keep this thread on-topic, you are most welcome to continuing discussing the pros/cons of AI assistants, but none of that addresses the questions raised by the OP.

Hi @crisconru - thanks for the interest. As the person who ultimately has to review and merge PRs, here's my response for you.

I'm aware the code base is in need of some housekeeping. There have been some contributions in that area, but they are hard to review properly due to the large amount of changes they bring. In some cases, the PRs have included bug fixes along the way - which makes it much harder to review as they now move the code around and change bits of logic in subtle ways.

If we can find a way to do this in a manageable way, then we should.

In terms of what would be in scope; ultimately I think the right answer will be for us in the core team to agree a set of standard linter rules and then incrementally apply them. These would mostly be stylistic things - formalising what's already there.

But we have to recognise this would be quite a disruptive piece of work to any inflight PRs. We need to decide how best to handle that.

The whole topic of ESM vs CJS is out of scope of this type of work - that would be a much larger piece of work to get right - with limited benefit (IMHO).

Where there are dependencies to be updated - they can be considered on a case-by-case basis.

1 Like

Thanks @knolleary for you response. Can I make a document for Core Team describing improvements / alternatives related to: linter, how to manage the monorepo, etc (kind of proposal) ? If that, how can I share with you? I could put here but maybe it could create offtopic conversations.

1 Like

The best route is to join the slack team (Redirecting…) and discuss in the #core-dev channel there.

1 Like

ok, I'll do that. Thanks again