Skip to content
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

use parseLink instead, and allow query on all refs #16

Merged
merged 2 commits into from
Apr 20, 2018

Conversation

mmckegg
Copy link
Contributor

@mmckegg mmckegg commented Apr 20, 2018

I hastily merged and published #15 as v2.10.0, but now I realise that @dominictarr was suggesting that we allow query on all ref types.

This PR adds support for handling query strings on all ref types, and adds a parseLink method which allows you to access this in the form of {link: "id...", query: {param: "value"}}

So I have unpublished 2.10.0 and submit this instead. The reason I unpublished is that this PR contains a breaking change to 2.10.0 where I'm using a generic parseLink instead of parseBlob.

@mmckegg mmckegg changed the title whoops, unpublished 2.10.0, use parseLink instead, and allow query on all refs use parseLink instead, and allow query on all refs Apr 20, 2018
the various `isBlob` `isFeed`, etc only detect the key itself, not the query. First parse with `parseLink` then give to ref.type to find out what type of link has been parsed

This no longer changes the behavior of any existing method.
@mmckegg
Copy link
Contributor Author

mmckegg commented Apr 20, 2018

Hmm, decided to change this so that it doesn't affect any existing methods.

the various isBlob isFeed, etc only detect the key itself, not the query. First parse with parseLink then give to ref.type to find out what type of link has been parsed

@mmckegg
Copy link
Contributor Author

mmckegg commented Apr 20, 2018

Okay, I'm going to merge this.

This is much safer than last time as it doesn't affect any existing code. All it does now is add a new function parseLink that handles parsing query strings on the end of ssb ref IDs.

@mmckegg mmckegg merged commit 41053a3 into master Apr 20, 2018
@mmckegg
Copy link
Contributor Author

mmckegg commented Apr 20, 2018

Released as ssb-ref@2.11.0 🎉

Hopefully less 👻 this time 😆

mmckegg added a commit to ssbc/ssb-mentions that referenced this pull request Apr 20, 2018
@dominictarr
Copy link
Contributor

@mmckegg 👍

I was a little confused because it looked like parseLink accepts a feed/msg/blob id with a ? then anything appended https://github.com/ssbc/ssb-ref/pull/16/files#diff-168726dbe96b3ce427e7fedce31bb0bcR5 ?

but then I saw it passes that to Querystring.parse

ssb-ref/index.js

Lines 161 to 170 in 680ed76

exports.parseLink = function parseBlob (ref) {
var match = parseLinkRegex.exec(ref)
if (match && match[1]) {
if (match[3]) {
return {link: match[1], query: Querystring.parse(match[4])}
} else {
return {link: match[1]}
}
}
}

hmm, I guess ?anything is a valid query string with an empty value

@dominictarr
Copy link
Contributor

am just wondering if this should be more restrictive, what shouldn't this permit? If we let something we don't want sneak in, we'll have to support it down the road if people start using it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants