Skip to content
This repository has been archived by the owner on Aug 29, 2023. It is now read-only.

chore: remove unnecessary wrap Socket with to-iterable #221

Closed
wants to merge 1 commit into from

Conversation

dapplion
Copy link
Contributor

Socket is already an async iterable since NodeJS v10 https://nodejs.org/api/stream.html#readablesymbolasynciterator

@achingbrain
Copy link
Member

That is handled by stream-to-it so I don't think this does any harm, though this PR has stopped the source being abortable by a passed in signal.

This is necessary because, for example, if we're dialling a peer as part of servicing an RPC request at a higher level, if that request is aborted before it's successful any resources created should be torn down, including sockets.

Is there a specific problem you'd observed that this fixes?

@dapplion
Copy link
Contributor Author

@achingbrain sorry PRs got mixed. Updated commit to the scope of the PR.

@achingbrain achingbrain changed the title Do not wrap Socket with to-iterable fix: do not wrap Socket with to-iterable Oct 19, 2022
@achingbrain
Copy link
Member

Did you check the linked repo? Because the socket has a [Symbol.asyncIterator] property it's returned from stream-to-it as a source without any wrapping or other funny business so I don't think this PR actually does anything. That is:

const { sink, source } = toIterable.duplex(socket)

console.info(source === socket) // true

@dapplion
Copy link
Contributor Author

Did you check the linked repo

Yes I did, this is why this PR exists. What's the point in calling a library that does not do anything if we know that all supported versions don't need wrapping?

@dapplion dapplion changed the title fix: do not wrap Socket with to-iterable chore: remove unnecessary wrap Socket with to-iterable Oct 28, 2022
@achingbrain
Copy link
Member

The code as it is is consistent and this PR doesn't fix a bug, improve performance or add a feature so I'm going to close this. Thanks for looking into it though.

@achingbrain achingbrain closed this Feb 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants