fix: loop over arguments instead of spreading

[bmeverett]

Description

Pushing a large number of arguments (100ks) to a command causes a RangeError: Maximum call stack size exceeded.

This is caused by using the spread operator since javascript can't hold all the values in memory. This updates to loop over the passed in array and add the values in a loop instead of using spread.

/app/node_modules/@node-redis/client/dist/lib/commands/generic-transformers.js:198
        args.push(...value);
             ^
RangeError: Maximum call stack size exceeded
   at pushVerdictArguments (/app/node_modules/@node-redis/client/dist/lib/commands/generic-transformers.js:198:14)
   at Object.transformArguments (/app/node_modules/@node-redis/client/dist/lib/commands/SREM.js:7:60)
   at transformCommandArguments (/app/node_modules/@node-redis/client/dist/lib/commander.js:62:23)
   at Commander.commandsExecutor (/app/node_modules/@node-redis/client/dist/lib/client/index.js:165:88)
   at Commander.BaseClass.<computed> [as sRem] (/app/node_modules/@node-redis/client/dist/lib/commander.js:8:29)
   at RedisService.removeKeysFromSetByPattern (/app/src/common/services/redis/redis.service.ts:232:23)
   at processTicksAndRejections (node:internal/process/task_queues:96:5)

---

Checklist

• Does npm test pass with this change (including linting)?
• Is the new or changed code fully tested?
• Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?

[CaptainCodeman]

I'm unfamiliar with the code, but wondered if the target is already an array, would .concat maybe perform better to save the looping?

[bmeverett]

@CaptainCodeman yea that's a good point. I've seen various arguments for using .concat over looping. Happy to use one over the other if there is a preference or performance boost.

[leibale]

https://perf.link/#eyJpZCI6ImFhcmJwcnhraG0xIiwidGl0bGUiOiJTcHJlYWQgVlMgQ29uY2F0IFZTIExvb3AiLCJiZWZvcmUiOiJjb25zdCBhcnIgPSBbXTtcbmZvciAobGV0IGkgPSAwOyBpIDwgMV8wMDBfMDAwOyBpKyspIHtcbiAgYXJyLnB1c2goTWF0aC5yYW5kb20oKS50b1N0cmluZygpKTtcbn0iLCJ0ZXN0cyI6W3sibmFtZSI6IlNwcmVhZCIsImNvZGUiOiJbLi4uYXJyXTsiLCJydW5zIjpbMjY0LDI5MCwyNzUsMjgzLDI2NywyODMsMzA5LDI4NiwzMTYsMzY5LDI1MiwyNzEsMjk0LDI3NSwzMDksMzAxLDI2NCwyNTIsMjY3LDI4MywyOTgsMjc1LDI3NSwyNzksMjc5LDI3NSwyNjcsMjUyLDMyNCwyNTYsMjYwLDI3NSwyNzEsMjkwLDI3NSwyOTAsMjU2LDI4MywyNzUsMjc1LDI3MSwyNzUsMjkwLDMxMywyODYsMjc5LDI4NiwyNzksMjg2LDI5MCwyNzksMjkwLDMwNSwyNjcsMjkwLDI2NCwyODYsMjcxLDI3NSwyODYsMzM1LDI3NSwyOTQsMjc1LDI3OSwyNDksMjcxLDI2MCwyNzUsMjk4LDI4MywzMjAsMjg2LDI3MSwyNzUsMjc1LDI2NCwyNzUsMjc1LDMwMSwyNzksMjk0LDM3NywyNzksMjcxLDI5NCwyNzUsMjk0LDMyMCwyNzUsMjc5LDI2NCwyMTUsMjc5LDI3OSwyODYsMjc5LDI0MSwzNTAsMjExXSwib3BzIjoyODF9LHsibmFtZSI6IkNvbmNhdCIsImNvZGUiOiJbXS5jb25jYXQoYXJyKTsiLCJydW5zIjpbMjcxLDI3MSwyODMsMjgzLDI2NywyODMsMzI0LDI4NiwzMzUsMzI0LDI3MSwyNTYsMjc5LDI3NSwzMTYsMzAxLDI2NywyNDksMjYwLDI4MywyOTAsMjg2LDI2NywyNzEsMjg2LDI2NCwyODMsMjYwLDM2NiwyNzEsMjY0LDI3MSwyNjAsMjkwLDI4MywyOTQsMjYwLDI4MywyNjQsMjY3LDI3MSwyNzEsMjc1LDM1OCwyNzEsMjc5LDI3OSwyNzUsMjc1LDMxMywyNjAsMjkwLDMzOSwyNTYsMzAxLDI3OSwyNzksMjcxLDI3NSwyNzksMzIwLDI3NSwyOTQsMjQ5LDI3OSwyMzMsMjk4LDI2MCwyNjcsMjg2LDI4MywyNzksMjc1LDMwOSwyNzksMjc1LDI2NCwyNzksMjgzLDI3NSwyNjQsMjkwLDMzMiwyOTAsMjk4LDI3MSwyNjcsMjgzLDI5OCwyNzEsMjgzLDI3OSwyMDcsMzA1LDI4NiwyNzEsMjg2LDI0OSwzNDMsMjAzXSwib3BzIjoyODB9LHsibmFtZSI6Ikxvb3AiLCJjb2RlIjoiY29uc3QgcmVzdWx0cyA9IFtdO1xuZm9yIChjb25zdCBpdGVtIG9mIGFycikge1xuICByZXN1bHRzLnB1c2goaXRlbSk7XG59IiwicnVucyI6WzI2LDI2LDIyLDI2LDI2LDIyLDIyLDI2LDI2LDI2LDIyLDIyLDI2LDI2LDI2LDIyLDI2LDI2LDI2LDI2LDIyLDI2LDI2LDI2LDMwLDI2LDIyLDI2LDI2LDI2LDI2LDIyLDI2LDIyLDI2LDI2LDIyLDI2LDI2LDI2LDIyLDI2LDI2LDI2LDIyLDI2LDI2LDI2LDIyLDI2LDI2LDI2LDI2LDI2LDI2LDI2LDIyLDI2LDI2LDIyLDI2LDI2LDMwLDIyLDI2LDIyLDIyLDI2LDI2LDI2LDI2LDI2LDI2LDI2LDI2LDI2LDIyLDI2LDI2LDI2LDI2LDIyLDMwLDI2LDIyLDI2LDI2LDIyLDI2LDI2LDI2LDI2LDIyLDIyLDIyLDIyLDI2LDIyLDIyLDIyXSwib3BzIjoyNH1dLCJ1cGRhdGVkIjoiMjAyMi0wNi0zMFQxNzowMzowNy4xNjBaIn0%3D

[bmeverett]

@leibale thanks for the link! Looks like spread and concat are pretty comparable so I updated to use concat

[bmeverett]

@leibale anything else I can do to help get this merged?

[leibale]

@bmeverett I'm not too sure if contact is actually better than spread, do you have any benchmarks (CPU or memory) that shows that?

[bmeverett]

@leibale so just to clarify, the main goal of this PR isn't necessarily performance, although I can see that it may come into play. I believe @CaptainCodeman was just asking about performance from my implementation.

The main issue is that spread doesn't work for large arrays (100ks) and throws the error I originally posted, Maximum call stack exceeded

MDN warns about using large values for spread.

When using spread syntax for function calls, be aware of the possibility of exceeding
the JavaScript engine's argument length limit. See apply for more details.

I am now using concat so that it works for large arrays instead of spread. If you're concerned about performance, we can implement some logic to only use concat for large arrays.

Happy to discuss further or clarify anything.

[leibale]

I'm also concerned that the spread implementation pushed to the original args argument, and concat creates a new array, which means that

const args = ['Hello'];
pushVerdictArguments(args, ['World']);
console.log(args);

will break (with the spread implementation args is ['Hello', 'World'], with the contact implementation args is ['Hello']).

I'll try to find another way to solve it (without decreasing performance) tomorrow.

[CaptainCodeman]

It just looked like the sort of thing where an inbuilt method might / should beat a loop, but from what I've read it seems that concat perf wins only when dealing with very large arrays. There's probably some cross-over point where it could use one or the other (?)

Another option to try would be batching the pushes as slices so there weren't as many loop iterations - no idea if it would be faster or not, the V8 engine may optimize the simple loop better than anything else.

[leibale]

@bmeverett review my changes please? 🙏

[bmeverett]

@leibale looks good to me! Thanks for the updates!