-
Notifications
You must be signed in to change notification settings - Fork 1
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
support encoding/decoding SSB URIs semantically #28
Conversation
staltz
commented
Aug 16, 2021
•
edited
Loading
edited
- Code reviews
- Pending on ssb-uri for bendybutt feed ssb-meta-feeds-spec#25 merged first
- Pending on adjust bfe.json so that only classic has sigils ssb-bfe-spec#20 merged first
- Pending on a new version of ssb-bfe-spec released on npm and updated here
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 message /msg thing is my main question.
Oh, and had thought about modifying the decide to to something like decode(buf, { toURI: true}) do you can force classic things to be uris too.
I think your solution of moving sigil seems wise though
@@ -6,7 +6,7 @@ const { bfeNamedTypes, bfeTypes } = bfe | |||
tape('00 feed type', function (t) { | |||
const values = [ | |||
'@6CAxOI3f+LUOVrbAl0IemqiS7ATpQvr9Mdw9LC4+Uv0=.ed25519', // classic | |||
'@6CAxOI3f+LUOVrbAl0IemqiS7ATpQvr9Mdw9LC4+Uv0=.bbfeed-v1', // bendy-butt | |||
'ssb:feed/bendybutt-v1/6CAxOI3f-LUOVrbAl0IemqiS7ATpQvr9Mdw9LC4-Uv0=', // bendy-butt |
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 diff makes it really nice and clear what's happening!
package.json
Outdated
@@ -16,7 +16,7 @@ | |||
}, | |||
"dependencies": { | |||
"is-canonical-base64": "^1.1.1", | |||
"ssb-bfe-spec": "^0.1.1", | |||
"ssb-bfe-spec": "github:ssb-ngi-pointer/ssb-bfe-spec#support-ssb-uris", |
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 do
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 you can upgrade this to the module latest now
@mycognosist I think this is a PR you could review, and I can drop arj (assuming he has too much to work on, but obviously if arj has comments I gladly welcome them too). |
@staltz this looks good to me, just needs module installing I think. |
Thanks @mixmix . I won't merge this PR before updating ssb-bfe-spec to an npm version, and before marking it non-draft. Just wanted more feedback (from more people) on the code, so that once ssb-bfe-spec is ready, we are ready to merge. |
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 can't spot anything obviously incorrect or out of place. Should be golden 👍🏻
Due to the tests being fundamentally changed (even if just a little), this will have be a new breaking change version. |