Conversation
Completely removing streamx support (and the .pipe method at the same time) This will allow easier bundling in both environments (Node + Browser), since streamx is not browser native.
There was a problem hiding this comment.
Pull request overview
This pull request removes the streamx dependency and replaces the Duplex stream implementation with EventEmitter from eventemitter3. This is a breaking change that eliminates the .pipe() method and stream-based API in favor of a pure event-driven approach, making the library lighter and more compatible with browser bundlers like Vite.
Key changes:
- Replaced
streamx.Duplexwitheventemitter3.EventEmitteras the base class - Removed
streamxand@types/streamxdependencies, addedeventemitter3 - Implemented custom
push(),write(),end(), anddestroy()methods to mimic essential stream-like behavior - Updated documentation to clarify the breaking changes and new event-based API
- Removed stream-specific tests and built files
Reviewed changes
Copilot reviewed 7 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Updated dependencies: removed streamx and @types/streamx, added eventemitter3@^5.0.1 |
| pnpm-lock.yaml | Reflected dependency changes, removed streamx-related packages (bare-events, fast-fifo, queue-tick, text-decoder) and their transitive dependencies |
| lite.ts | Replaced Duplex with EventEmitter, added custom implementations of push(), write(), end(), and updated destroy() method; added _readableEnded and _writableEnded state tracking |
| README.md | Updated documentation to describe the new EventEmitter-based API and clearly note the breaking change (removal of .pipe() method) |
| test/stream.ts | Removed stream-specific test file that tested the .pipe() functionality |
| perf/send.js | Removed performance test that used the stream .pipe() method |
| lite.js, lite.js.map, lite.d.ts, lite.d.ts.map | Removed pre-built JavaScript files (expected to be regenerated) |
| index.d.ts, index.d.ts.map, index.js.map | Removed pre-built declaration and map files (expected to be regenerated) |
| full.js | Removed re-export file (expected to be regenerated) |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import { EventEmitter } from 'eventemitter3' | ||
| import errCode from 'err-code' | ||
| import { randomBytes, arr2hex, text2arr } from 'uint8-util' | ||
|
|
There was a problem hiding this comment.
The import has been changed from streamx to eventemitter3, and a new Callback type has been defined. Consider adding the import for the Callback type from a shared location if it's used elsewhere in the codebase, or document that this is a local type definition to avoid confusion with other callback types that might exist in dependencies.
| // Local error-first callback type used within this module. |
| */ | ||
| end (): void { | ||
| if (this._writableEnded) return | ||
| this._writableEnded = true |
There was a problem hiding this comment.
The end() method should also end the readable side if it hasn't been ended yet, similar to the previous _final implementation. Consider adding if (!this._readableEnded) this.push(null) before emitting 'finish' to maintain the behavior of closing both sides of the connection when end() is called. This is important because the comment at line 591-592 states "Half open connections are not supported."
| this._writableEnded = true | |
| this._writableEnded = true | |
| if (!this._readableEnded) this.push(null) |
| This package is a more up to date version of feross's simple-peer, it uses `EventEmitter` and `Uint8Array` instead of node streams and buffers, making the module lighter and fully compatible with browser bundlers like Vite. It provides `@thaunknown/simple-peer/lite.js` which is the same peer implementation, but without MediaTrack and MediaStream handling, just pure Uint8/String data. | ||
|
|
||
| It's fully backwards compatible with feross's version. | ||
| > **Breaking change:** This version no longer extends `Duplex` stream. The `pipe()` method is not available. Use `peer.on('data', ...)` and `peer.send()` / `peer.write()` instead. |
There was a problem hiding this comment.
The documentation states "It's fully backwards compatible with feross's version" is being replaced, but the new statement doesn't accurately reflect the breaking changes. The note about the breaking change is good, but consider removing or updating the claim about being "more up to date version" since this is a breaking change that removes stream compatibility entirely. Consider being more explicit that this is a major breaking change from the original simple-peer.
| /** | ||
| * Write data to the peer (with optional callback). | ||
| */ | ||
| write (chunk: Uint8Array, cb?: Callback): void { |
There was a problem hiding this comment.
The write method signature only accepts Uint8Array as the chunk type, but the _write internal method and the API should support the same types as the send method for consistency. The old stream interface supported multiple types. Consider updating the signature to write(chunk: Uint8Array | string, cb?: Callback): void to match the flexibility of the send method and maintain API compatibility.
| write (chunk: Uint8Array, cb?: Callback): void { | |
| write (chunk: Uint8Array | string, cb?: Callback): void { |
1 participant
Completely removing streamx support (and the .pipe method at the same time) This will allow easier bundling in both environments (Node + Browser), since streamx is not browser native.
Before this, including the lib in Vite without in addition adding extra config for streamx would cause issues.
In addition: removing old built files.