Code smells

[rotu]

Is there an existing issue for this?

• I have searched the existing issues

Current Behavior

The following code is slightly suspect. If written as intended, it could use a comment.

Due to the way once works, this expression throws an Array containing a single Error instead of the probably intended Error.

agent/lib/agents.js

Lines 126 to 128 in 4b2c4d1

	(s) => once(socket, 'error', s).then((err) => {
	throw err
	}),

This code creates a non-resolving promise (maybe instead of an immediately-resolving Promise?).

agent/lib/util.js

Line 23 in c51096b

return signal ? new Promise(() => {}) : null

Initially reported when trying to figure out the root cause behind: npm/cli#6763

Expected Behavior

No response

Steps To Reproduce

1. In this environment...
2. With this config...
3. Run '...'
4. See error...

Environment

• npm:
• Node:
• OS:
• platform:

[lukekarrys]

Due to the way once works, this expression throws an Array containing a single Error instead of the probably intended Error.

This is a mistake and should be fixed. Nice catch!

This code creates a non-resolving promise (maybe instead of an immediately-resolving Promise?).

This could use a comment, or a refactor. This code path is only used in a Promise.race so passing a non-resolving promise is done on purpose if no timeout is passed in. Using an immediately resolving promise would make it immediately resolve the Promise.race which is not what is wanted.

[wraithgar]

If the promise never resolves, what is it racing?

[wraithgar]

oh ... if no timeout is passed. We should just return the original promise then.

ETA I think a bit of a refactor is in order here too. These are good catches!

[rotu]

I figured the immediate resolution was intended, since that's also the behavior for when delay is 0.

Also, the createTimeout returns either null, a non-resolving Promise, a {start, clear} object, or an inscrutable wrapper promise. And it has an argument called signal which seems to be given an AbortController, not an AbortSignal?

This code is an impressive fractal of WTF!

[hashtagchris]

Thanks @rotu! Core feedback addressed.

First array element is now thrown:

agent/lib/agents.js

Lines 149 to 151 in d3187c4

	once(socket, 'error', { signal }).then((err) => {
	throw err[0]
	}),

util.js has been removed, and I couldn't find any non-resolving Promises.