-
Notifications
You must be signed in to change notification settings - Fork 443
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
feat: add fetch protocol #1036
feat: add fetch protocol #1036
Conversation
src/identify/message.d.ts
Outdated
@@ -34,25 +34,40 @@ export class Identify implements IIdentify { | |||
constructor(p?: IIdentify); |
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.
all these changes to the random autogenerated protobuf code happened when I ran npm run build:proto && npm run build:proto-types
on a totally clean checkout before I even started making any of my changes.
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.
If you want me to leave those changes out of this PR just let me know and I can unstage those files.
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.
Yes please!
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.
Yep will do. I'm still worried though that the code generated for the new fetch protocol protobufs is going to have the same problems as clearly there's something in my local setup causing it to generate protobufs differently than what yall have here.
@vasco-santos @achingbrain this is needed to implement the updated IPNS over PubSub spec (from 2019) and might also be useful for other folks such as Ceramic who just need a basic "give me the data corresponding to You may already be doing this, but the main technical note I'd give is that you should give room to pull from multiple data sources. Side note: @stbrody I'm also realizing that it might be useful to work on a version of the |
Yeah I thought about this a little bit. The API right now is very simple - when the protocol is registered the user provides a callback to map key to value. If an app developer wanted to use two different subsystems that both relied on the fetch protocol, the dev could easily wrap the two lookup functions from each subsystem into a single one that dispatches based on the key. I feel like if we try to build some way to register multiple lookup callbacks to different ranges of the keyspace, it's just going to complicate the API and may wind up being too brittle and prescriptive for every use case. I think it might genuinely make sense to leave this to users, but I could be swayed if you disagree @aschmahmann EDIT: actually I thought of a way that isn't too bad - if we force each subsystem to have a fixed prefix on all keys (instead of trying to do something like a range match or regex which would be too complicated I think). I can give a try to making that update when I'm back from vacation next week. |
I seem to be getting failures generating type information, but the errors are all coming from the autogenerated protocol buffer code: https://github.com/libp2p/js-libp2p/runs/4316690228?check_suite_focus=true I'm wondering if this is related to why a bunch of autogenerated protobuf code got updated when I ran the |
Okay, updated to be more modular and allow registering multiple key lookup functions for different fixed prefixes of the keyspace. Hopefully this addresses your main concern @aschmahmann |
@aschmahmann, I had an idea that I think would make this a bit simpler to use and understand. Instead of having a key prefix to dispatch request handlers, what if we added a new string field |
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.
This is great, thanks so much for submitting it. Just a few minor comments for consistency.
The implementation needs to be mounted in https://github.com/libp2p/js-libp2p/blob/master/src/index.js and exposed on the libp2p
class to let consumers configure lookup functions - see how .indentifyService
and the ping protocol are configured for an example.
The generated .js
file also needs to be added to the exclude list in https://github.com/libp2p/js-libp2p/blob/master/tsconfig.json - this plus reverting the changes to the generated files should get linting passing in CI.
Finally please merge master/rebase as it's had some changes made that make the build a lot more stable.
Ideally some tests should be added to https://github.com/libp2p/interop to ensure it works across implementations but I don't know how feasible that is as it uses https://github.com/libp2p/go-libp2p-daemon which is a bit out of date.
5824285
to
d658007
Compare
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.
Okay, ready for another round of review. I reverted the auto-generated protobuf code, merged in master, and added error codes. PTAL
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.
Thanks so much for adding this. I've tided some things up to make it more consistent with the existing code, added some extra tests and docs, and exposed the .fetch
command as a top-level libp2p method.
I've also changed the protocol name to be consistent with go-libp2p, otherwise we won't have interop.
@stbrody this is now available in libp2p under the $ npm install libp2p@next |
## [0.36.0](https://www.github.com/libp2p/js-libp2p/compare/v0.35.8...v0.36.0) (2022-01-25) ### ⚠ BREAKING CHANGES * abort-controller dep is gone from dependency tree * `libp2p.handle`, `libp2p.registrar.register` and the peerstore methods have become async ### Features * add fetch protocol ([#1036](https://www.github.com/libp2p/js-libp2p/issues/1036)) ([d8ceb0b](https://www.github.com/libp2p/js-libp2p/commit/d8ceb0bc66fe225d1335d3f05b9a3a30983c2a57)) * async peerstore backed by datastores ([#1058](https://www.github.com/libp2p/js-libp2p/issues/1058)) ([978eb36](https://www.github.com/libp2p/js-libp2p/commit/978eb3676fad5d5d50ddb28d1a7868f448cbb20b)) * connection gater ([#1142](https://www.github.com/libp2p/js-libp2p/issues/1142)) ([ff32eba](https://www.github.com/libp2p/js-libp2p/commit/ff32eba6a0fa222af1a7a46775d5e0346ad6ebdf)) ### Bug Fixes * cache build artefacts ([#1091](https://www.github.com/libp2p/js-libp2p/issues/1091)) ([5043cd5](https://www.github.com/libp2p/js-libp2p/commit/5043cd56435a264e83db4fb8388d33e9a0442fff)) * catch errors during identify ([#1138](https://www.github.com/libp2p/js-libp2p/issues/1138)) ([12f1bb0](https://www.github.com/libp2p/js-libp2p/commit/12f1bb0aeec4b639bd2af05807215f3b4284e379)) * import uint8arrays package in example ([#1083](https://www.github.com/libp2p/js-libp2p/issues/1083)) ([c3700f5](https://www.github.com/libp2p/js-libp2p/commit/c3700f55d5a0b62182d683ca37258887b24065b9)) * make tests more reliable ([#1139](https://www.github.com/libp2p/js-libp2p/issues/1139)) ([b7e8706](https://www.github.com/libp2p/js-libp2p/commit/b7e87066a69970f1adca4ba552c7fdf624916a7e)) * prevent auto-dialer from dialing self ([#1104](https://www.github.com/libp2p/js-libp2p/issues/1104)) ([9b22c6e](https://www.github.com/libp2p/js-libp2p/commit/9b22c6e2f987a20c6639cd07f31fe9c824e24923)) * remove abort-controller dep ([#1095](https://www.github.com/libp2p/js-libp2p/issues/1095)) ([0a4dc54](https://www.github.com/libp2p/js-libp2p/commit/0a4dc54d084c901df47cce1788bd5922090ee037)) * try all peer addresses when dialing a relay ([#1140](https://www.github.com/libp2p/js-libp2p/issues/1140)) ([63aa480](https://www.github.com/libp2p/js-libp2p/commit/63aa480800974515f44d3b7e013da9c8ccaae8ad)) * update any-signal and timeout-abort-controller ([#1128](https://www.github.com/libp2p/js-libp2p/issues/1128)) ([e0354b4](https://www.github.com/libp2p/js-libp2p/commit/e0354b4c6b95bb90656b868849182eb3efddf096)) * update multistream select ([#1136](https://www.github.com/libp2p/js-libp2p/issues/1136)) ([00e4959](https://www.github.com/libp2p/js-libp2p/commit/00e49592a356e39b20c889d5f40b9bb37d4bf293)) * update node-forge ([#1133](https://www.github.com/libp2p/js-libp2p/issues/1133)) ([a4bba35](https://www.github.com/libp2p/js-libp2p/commit/a4bba35948e1cd8dbe5147f2c8d6385b1fbb6fae)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
All changes look great, thanks @achingbrain!!! I'm excited to try this out. Hopefully I'll have time to check it out in the upcoming 2 week sprint. I'm still curious though what you and @aschmahmann think about the idea of using an explicit |
No description provided.