sindresorhus / p-reduce

Reduce a list of values using promises into a promise for a value
MIT License
68 stars 7 forks source link

`no-array-reduce`? #6

Closed fregante closed 1 year ago

fregante commented 2 years ago

-> https://github.com/sindresorhus/eslint-plugin-unicorn/blob/v40.0.0/docs/rules/no-array-reduce.md

Does this package still make sense? Should it be deprecated?

p-reduce

import pReduce from 'p-reduce';
import humanInfo from 'human-info'; // Not a real module

const names = [
    getUser('sindresorhus').then(info => info.name),
    'Addy Osmani',
    'Pascal Hartig',
    'Stephen Sawchuk'
];

const totalAge = await pReduce(names, async (total, name) => {
    const info = await humanInfo(name);
    return total + info.age;
}, 0);

console.log(totalAge);
//=> 125

for of

import humanInfo from 'human-info'; // Not a real module

const names = [
    getUser('sindresorhus').then(info => info.name),
    'Addy Osmani',
    'Pascal Hartig',
    'Stephen Sawchuk'
];

let totalAge = 0;
for (const name of names) {
    const info = await humanInfo(name);
    totalAge += info.age;
}

console.log(totalAge);
//=> 125
sindresorhus commented 2 years ago

It's only somewhat useful in the rare case of summing up numbers, which is allowed by default.

Which is what the example in the readme is doing.

fregante commented 2 years ago

I think not? There's a constant assignment and an await in there. It looks like a regular reduce loop to me, not the simple (t, b) => t+b exemption.

sindresorhus commented 2 years ago

The example shows summing up age, which is summing up a number, which is the only valid use-case for reduce.

fregante commented 2 years ago

Why is the only valid use-case? It's because "it does not require return" in practice, not just because it's a sum. I don't see a difference between:

const totalAge = await pReduce(names, async (total, name) => {
    const info = await humanInfo(name);
    return total + info.age;
}, 0);

and

const notTheTotal = await pReduce(names, async (total, name) => {
    const info = await humanInfo(name);
    return totallyNotSum(total, info.age);
}, {});

The amount of code and readability is equal IMO. The exception is only valid if the code were:

const notTheTotal = await pReduce(
    names,
    async (total, name) => total + await humanAge(name),
    0
);

and even then it's kind of a stretch because it's looping things in series when it could totally get humanAge in parallel. So not only is this package against no-array-reduce but also no-await-in-loop

Better yet:

// Parallel sum
const ages = await Promise.all(names.map(n => humanAge(n)));
const total = ages.reduce((t, a) => t + a)

or exclusively this, which isn't a huge improvement over the last example:

const ages = names.map(n => humanAge(n));
const total = await pReduce(ages, (t, a) => t + a)
sindresorhus commented 2 years ago

I'm talking about reduce in general, not what the lint rule is able to detect.

fregante commented 2 years ago

I'm not talking about the linter either, I'm just saying that the reasons for which no-array-reduce exists are the same for this package to not exist:

additionally, the sum exception:

furthermore:

sindresorhus commented 2 years ago

reduce is unreadable other than for sums

But the package literally recommends using it for sums.

should not apply to the readme example because it's a block function and has poor readability just like any other reduce PoCs

I disagree. The example is pretty clear.

is only applicable if you're reducing an Array<Promise>, which saves just about 4 characters

I do however agree that it's solving a very obscure problem and it's more likely to be abused for other things.

most examples go against no-await-in-loop too

Not sure what you mean? It's summing concurrently.

sindresorhus commented 2 years ago

In short. I disagree with some of your reasoning. But I agree with deprecating this package.

sindresorhus commented 1 year ago

https://github.com/sindresorhus/p-reduce/commit/c5777d0deebe0376138210ffae9011c8530d069d

vladshcherbin commented 1 year ago

It's quite amazing to read this message and then come across the p-waterfall source code, which is clearly not intended for performing number sum 😅

btw The note is not visible in npm readme since it was added after the latest release.