-
Notifications
You must be signed in to change notification settings - Fork 3
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 DB2 #9
Support DB2 #9
Conversation
Didn't ssb-backlinks or ssb-friends still need to be ported to db2? |
Yeah, here's the relevant bug: |
ssb-friends: yes. Luckily its hidden behind a if so it should work without. ssb-backlinks: is only for db1. So all the code should use db.query instead when running against sbot.db (db2). |
Okay, cool. Was just reading through the documentation to try to figure out how to use these properly and noticed that as a dependency. |
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.
https://github.com/ssbc/ssb-suggest/blob/db2/lib/watch.js#L111
Typo here: this should be howFarBack.
(Sorry - I don't really know how to do reviews of specific lines on here.) |
Also needs toPullStream() added to the query. After much fiddling, this is the version I came up with which finally does not crash:
I will admit that I have a hard time grokking pullstreams. But the way it was kept crashing with complaints about msg.value.author not existing, and it turns out it that the map() function was being passed an array of all the messages instead of just one at a time. So I got rid of that and just did it via a loop. |
Evidently even that crashes sometimes. Might need something like this:
|
lib/watch.js
Outdated
ssb.db.query( | ||
live(), // old: true | ||
toPullStream() | ||
), |
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.
How about a pullCat
here of the two parts? (line 112 and line 126)
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.
Wanted to combine them into one, but it almost seemed as if it was simpler with 2 in the end. I'm quite open to changing this.
lib/watch.js
Outdated
|
||
pull( | ||
ssb.db.query( | ||
and(type('post')), |
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.
Is this really type('post')
correct?
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.
Would be better with a general query for all kinds of messages. But I wanted something that worked. A bit tired 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.
Tired too. Let's talk tomorrow
One note from testing is that the timeouts seems somewhat excessive. It takes a while for the suggestions to be ready. Maybe we can lower that for db2. |
Pushed up a new commit. I have tested this locally and it works great :) @staltz final review? |
toPullStream() | ||
), | ||
pull.take(1) | ||
), |
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.
There are some problems here, and mostly due to line 111. I think we could just remove 111 because we have the pullCat with line 118.
Currently what this code means:
- 1st ssb.db.query is "fetch
howFarBack
old msgs in descending order, then stop" - 2nd ssb.db.query is "fetch all new live msgs in ascending order"
The live in the 1st doesn't get to run because of the pull.take(1) that cuts the execution of the 1st pull-stream.
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 tested that it worked in both branches. Meaning we get the first 300 then all live updates. I think it would be easier to read without the live in the first one. Pushed up a commit for that.
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 it did seem to work just by looking at the code. But given that the semantics were slightly off, maybe a corner case bug could have slipped by. Thanks for changing it!
By the way, I got curious what's the fastest way of iterating over arrays, and I ran this: https://jsbench.me/rckki47i41/2 (Prompted by your use of arr.forEach. We don't need to change it because it's a small array, 300 items, I just wanted to know what's the perf profile) On my computer, left is Firefox, right is Chrome: Summary:
|
@KyleMaas this is one of the modules we need to get profiles (about messages) working in properly with db2. This module can replace the getProfiles in browser in cases where you can select people like when writing private messages.
Please note this PR is totally untested! I'm hoping you can try and plug it into ssb-browser-core together with ssb-about and use this instead of the about-profile index we have been using for testing purposes. This depends on ssbc/ssb-social-index#4.