-
Notifications
You must be signed in to change notification settings - Fork 506
feat!: merge stat properties into stream/connection objects #1856
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
Conversation
|
Extracted from #1847 |
interop/test/ping.spec.ts
Outdated
| break | ||
| case 'yamux': | ||
| options.streamMuxers = [yamux()] | ||
| options.streamMuxers = [mplex()] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this crept in
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this breaks the Stream interface, which@chainsafe/yamux has a dependency on, but modules here depend on @chainsafe/yamux creating a circular dependency. @libp2p/mplex here is updated to the new version but @libp2p/interface will need a release that @chainsafe/yamux can be upgraded to.
Groan.
I guess I could include it in this PR as @libp2p/yamux or something then we can swap out @libp2p/yamux for @chainsafe/yamux after the release?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh darn. I guess this is the issue with relying on packages outside the monorepo when doing a breaking change.
At least you could make the diff smaller with somehting like:
// TODO replace this import once @chainsafe/libp2p-yamux has been updated
import { mlex as yamux } from '@libp2p/mplex'There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've temporarily added yamux to the monorepo but with "private": true" in the package.json, this should let us build and publish the other packages without trying to publish yamux, we can then open a PR to yamux afterwards with the changes and remove it from here.
Moves `direction`, `timeline`, `status`, etc from `.stat` objects on `Stream` and `Connection` objects onto the root object so we don't have awkward constructs like `conn.stat.status`, instead the simpler `conn.status`. Fixes #1849 BREAKING CHANGE: `stream.stat.*` and `conn.stat.*` properties are now accessed via `stream.*` and `conn.*`
dffd58d to
39e5767
Compare
d5f1937 to
8242b2d
Compare
maschad
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am bit confused, what necessitates the addition of libp2p-daemon and libp2p-daemon-client and libp2p-daemon-server here?
|
|
||
| ## API Docs | ||
|
|
||
| - <https://libp2p.github.io/js-libp2p/modules/multidim_interop.html> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps API DOCS isn't the most appropriate header but we should link to the interop repo since that's where the bulk of the code lives
|
The daemon has a dependency on libp2p (e.g. it references the changed stream properties), but libp2p uses the daemon in it’s interop tests so 🫠 |
Moves
direction,timeline,status, etc from.statobjects onStreamandConnectionobjects onto the root object so we don't have awkward constructs likeconn.stat.status, instead the simplerconn.status.Fixes #1849
BREAKING CHANGE:
stream.stat.*andconn.stat.*properties are now accessed viastream.*andconn.*