Skip to content

Node v8 stream destroy and _destroy methods #187

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
mwiktorczyk opened this issue Jun 29, 2017 · 2 comments · Fixed by #237
Closed

Node v8 stream destroy and _destroy methods #187

mwiktorczyk opened this issue Jun 29, 2017 · 2 comments · Fixed by #237

Comments

@mwiktorczyk
Copy link

Node v8 introduced some changes to stream API. One of them is destroy method:

According to this specification,

implementors should not override this method, but instead implement readable._destroy

Is there a chance to adapt your library to node v8 and later on fix the TypeScript definitions?

@t-mullen
Copy link
Collaborator

t-mullen commented Nov 1, 2017

readable-stream doesn't seem to implement the same behaviour. (calling .destroy doesn't call the overridden ._destroy)

It seems it would be as simple as removing Peer.prototype.destroy if this was the case.

One thing to consider is that the Node V8 .destroy method isn't documented as having a callback parameter, but takes an error. We do the opposite: Peer.prototype.destroy(onclose)

https://nodejs.org/api/stream.html#stream_writable_destroy_error

feross added a commit that referenced this issue Feb 17, 2018
Node.js 8 standardized the destroy() stream interface, so let's use that interface.

To update your code, change this:

```js
peer.destroy(callback)
```

To this instead:

```js
peer.on('close', callback)
peer.destroy()
```

Fixes: #187
@feross
Copy link
Owner

feross commented Feb 17, 2018

I think we can switch to the new interface and just shim the missing .destroy() method until this issue is fixed: nodejs/readable-stream#283

This is a breaking change since it changes the API, but it makes the API match Node.js which is aligns with user expectations and the stated goal of this package (to emulate net.Socket where possible).

PR here: #237

FredZeng pushed a commit to FredZeng/simple-peer that referenced this issue Oct 14, 2023
Node.js 8 standardized the destroy() stream interface, so let's use that interface.

To update your code, change this:

```js
peer.destroy(callback)
```

To this instead:

```js
peer.on('close', callback)
peer.destroy()
```

Fixes: feross#187
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants