-
Notifications
You must be signed in to change notification settings - Fork 105
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
fix: add helia version to agent version #128
Conversation
If the user has configured identify but not overidden the agent version, add helia and its version number to the string.
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.
lgtm but confused why we're saving version & name in version.ts
instead of version.json
or a more aptly named agentInfo.json
export const version = '0.0.0' | ||
export const name = 'helia' |
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.
why not have this file be a json file
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.
Node 18 still shows a warning if you import json from js:
(node:25810) ExperimentalWarning: Importing JSON modules is an experimental feature and might change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
const agentVersionBuf = peer.metadata.get('AgentVersion') | ||
const agentVersion = new TextDecoder().decode(agentVersionBuf) |
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.
does it make sense to maintain and expose a method for obtaining a decoded agent version ? I can imagine having to decode this agent version being a minor pain point for users.
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.
The AgentVersion is used by the identify protocol and it's not something people generally interact with directly.
🎉 This PR is included in version @helia/interface-v1.1.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
🎉 This PR is included in version helia-v1.1.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
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 know there are tests/aseerts here (great - thanks). For my own learning, could you please paste what the resulting full "versionStr" is that Helia will now be using (and what it was before). (I know I could get things up and running to figure out myself - I'm asking as a time saver for me - thank you!)
if (versionStr.match(/js-libp2p\/\d+\.\d+\.\d+\sUserAgent=/) == null) { | ||
// the user changed the agent version |
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'm not saying we need to open a new PR, but I think it would be nice in future for these kinds of things if we linked to the source of the information (presumably somewhere in js-libp2p).
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.
Agree, but it’s buried fairly deeply in the identify service so would need a change there - it’s not something that’s needed to be accessed before.
Before:
Where After:
|
Thanks for the info @achingbrain . I added ann FAQ entry: https://github.com/ipfs/helia/wiki/%E2%9D%93-FAQ#-how-does-helia-identify-itself. Please modify if I got anything wrong. |
If the user has configured identify but not overridden the agent version, add helia and its version number to the string.
Fixes #122