stream: move duplicated code to an internal module

[Trott]

isIterable() is called in one place with only one argument, so the
second argument and associated logic for it can be removed.

Create a utils module for isIterable(), isReadable(), and isStream().

[RaisinTen]

@Trott isIterable was added in #37359 probably in the style of #31223 which added isIterable here too:

node/lib/internal/streams/pipeline.js

Lines 97 to 103 in 448de26

	function isIterable(obj, isAsync) {
	if (!obj) return false;
	if (isAsync === true) return typeof obj[SymbolAsyncIterator] === 'function';
	if (isAsync === false) return typeof obj[SymbolIterator] === 'function';
	return typeof obj[SymbolAsyncIterator] === 'function' ||
	typeof obj[SymbolIterator] === 'function';
	}

What do you think about exporting it from lib/internal/streams/pipeline.js and importing and using it here instead?
That way we could avoid some code duplication too.

[RaisinTen]

Removing the author ready label so that my comment gets addressed before landing this PR.

[Trott]

@RaisinTen I think we can get rid of isIterable() entirely. Looking at the only place where it is used, there's more unreachable/duplicate code than just what is currently removed in this PR. The first thing it does is check to see if obj is a falsy value, but that's already covered before isIterable() is invoked:

if (lastArg && typeof lastArg === 'object' &&
        !isStream(lastArg) && !isIterable(lastArg))

Then there are the lines removed in this PR because they rely on a second argument that is never passed.

That leaves this line as the only thing that isIterable() actually does here:

return typeof obj[SymbolAsyncIterator] === 'function' ||
    typeof obj[SymbolIterator] === 'function';

It seems to me that maybe it's better to inline those two checks rather than import isIterable()?

[Trott]

It seems to me that maybe it's better to inline those two checks rather than import isIterable()?

On the other hand, that does make it less obvious what is going on....

[Trott]

I'm not sure that pipeline.js is the right place to export utility functions like isIterable(). I think it would be better to move those to a utils.js somewhere or something like that?

And if we import isIterable() from pipeline.js (or a utils.js), we'll also want to do the same for isReadable(), isWritable(), and isStream(). So maybe they can all go in a lib/internal/streams/utils.js or something like that?

[Trott]

And if we import isIterable() from pipeline.js (or a utils.js), we'll also want to do the same for isReadable(), isWritable(), and isStream(). So maybe they can all go in a lib/internal/streams/utils.js or something like that?

OK, this is what I did. PTAL.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/36391/

[Trott]

@nodejs/streams

[mcollina]

lgtm

[Trott]

Landed in 1e34df1