main located at root/v1/:shard/main branch #94
Conversation
staltz
changed the title
|
Good idea. Will review logic closely when at computer |
| debug('adding main feed to root meta feed') | ||
| const opts = optsForAddExisting(mf.keys, 'main', config.keys) | ||
| debug('adding main feed to a shard metafeed') | ||
| const opts = optsForAddExisting(shardFeed.keys, 'main', config.keys) |
There was a problem hiding this comment.
🔥 / 🌶️
If I understand this right, this is announcing the location of the feed structure on the classic main feed, right?
If that's the case I would like to recommend that we still announce the rootMF, as it's much more important to know than some shard feed right?
It might be that that's not really how metafeed/add/existing works, but we should perhaps consider include the rootMF as meta-data then...
There was a problem hiding this comment.
I think you understood it the other way around. What this line does is that it announces the main feed as a subfeed of the shard, by publishing a message on the shard.
There was a problem hiding this comment.
Oh I thought that would just be a meta/add/existing on the sharf feed which points to a main feed.
I'm assuming there is a message published on the main feed that points back to the root feed (to support people migrating)
There was a problem hiding this comment.
Yes there is, check the code block above this block.
There was a problem hiding this comment.
Only one thing I think we "must" consider before merging (see 🔥 / 🌶️)
Feedback: there were two changes made in this PR which could have been seperated into 2 PRs to smooth reviewing:
- move the main subtree to
root/v1/:shard/main - prettier changes (this changed the formatting of lots of tests which forced me to read closely because you were also changing the API at the same time)
Tone: I'm not being smug, I'm genuinely trying to embody what I think we said we both wanted to value (PRs that are easy to review and merge, and code that doesn't have regressions). I am enjoying noticing the pain you've pointed out to me. Interested if you're giving me a taste of it to inoculate me, or if you didn't notice yourself 🤣 ❤️
This is a good PR, love this tuning
@arj03 we need to make sure this change is recorded in the "migration" spec
Yeah, you're right
Indeed! |
|
I addressed the chili by answering your comment, and then I did the code style changes. |
|
If I misunderstood that announce thing that's fine. Main thing I wanted to check was that we hadn't missed a side effect of changing where and how we attach the main feed. The mobile ui sucks, so: approve 👍 |
Context
I'm working on ssb-replication-scheduler to make it understand the v1 tree structure, and it expects all feeds to be in paths
root/v1/:shard/___.Problem
ssb-meta-feeds was still doing
root/main. See also ssbc/ssb-meta-feeds-spec#35Solution
When the root metafeed is created, publish the main as a
metafeed/add/existingunderroot/v1/:shard/main.1st ❌ 2nd ✔️