search

ceramicnetwork / js-ceramic

Typescript implementation of the Ceramic protocol
http://ceramic.network
Other
414 stars 127 forks source link

feat: FeedStream #3216

Closed ukstv closed 5 months ago

linear[bot] commented 5 months ago

WS2-3118 FeedStream

JulissaDantes commented 5 months ago

seems like this still needs a lot of test coverage, especially around the resumability behavior and the http-api. Is that coming in a separate PR?

Yes @stbrody , this one: https://github.com/3box/ceramic-tests/pull/116

stbrody commented 5 months ago

seems like this still needs a lot of test coverage, especially around the resumability behavior and the http-api. Is that coming in a separate PR?

Yes @stbrody , this one: 3box/ceramic-tests#116

hmm... it feels kind of odd to me to have so little coverage in the actual js-ceramic repo, and to push all the test coverage out to the ceramic-tests repo... I guess the biggest concern though is PR gating, and I believe we have plans to start gating merges to js-ceramic on the fast tests from ceramic-tests, so I guess once that happens that will address the biggest downside of the separation. Although I do think it will still be a bit harder to rapidly iterate on tests and code and do proper test-driven development when the tests live in a different repo.

Certainly we could write all these tests in the stream-tests package within the js-ceramic repo. That way they would run in CI today, and be fast and easy to run and iterate on for local development, but they'd miss out on the ability to run against our real infra. Ideally I feel like we'd have tests in both places, but that's probably overkill right now. So if we only write the tests once, I'm conflicted on which place is better. I'm curious what @nathanielc and @smrz2001 think about the tradeoffs here.

ukstv commented 5 months ago

Okay, @stbrody @JulissaDantes I got back the relevant tests migrated from Tiles to MIDs. As for the time and gte, I am inclined to go with gt but with a nanoseconds cursor. It is effectively impossible to have same nanoseconds for different writes.

ukstv commented 5 months ago

BTW, image verification is due to a weird blockchain JSON RPC error, seemingly unrelated to this change.

ukstv commented 5 months ago

generally looking good, except I still feel pretty strongly that we should use GTE on the resume query. Very unlikely is still possible, and I'd rather us just know for sure that we'll never miss an event rather than risk an edge case that can result in data loss.

I would like not to play telepathy here. If you have a solid proposal in mind, please, write it down.

ukstv commented 5 months ago

Okay @stbrody here is a fancy counter we agreed upon: https://github.com/ceramicnetwork/js-ceramic/pull/3216/files#diff-286f50cb14ccba5132a33f458e81c0a5798d5f910b518810f3d16b1f9dacc135R28 Same millisecond entries get deduplicated.