feat: add redis adapter #304
Conversation
kid-icarus
force-pushed
the
redis-adapter
branch
2 times, most recently
from
6eb0c27
to
7c849ba
Compare
kid-icarus
force-pushed
the
redis-adapter
branch
from
7c849ba
to
0a35bb0
Compare
|
This looks good to me, but the only reason we know the existing adapters work is because we are using them in production! As you mention, there aren't any specific tests for any of the Storage Adapters, and it would in indeed be good to have a standard set for all of them. Without being in a position to test it myself, I am loathe to merge just yet. I imagine this will be something people are looking for though. |
|
I think as the system matures we ought to start putting the storage & network adapters in their own repos (though then we lose automated test integration... hmm) but this is definitely something I've wanted to see for a while so thanks for sharing it! |
| @@ -0,0 +1,19 @@ | |||
| Copyright (c) 2024-Present, Ryan Kois | |||
There was a problem hiding this comment.
Looks like in this file you missed the first line. Like here.
| "dependencies": { | ||
| "@automerge/automerge-repo": "workspace:*", | ||
| "debug": "^4.3.4", | ||
| "ioredis": "^5.3.2" |
There was a problem hiding this comment.
It feels like the ioredis must be peer dependency. What do you think?
There was a problem hiding this comment.
Yeah that makes sense, I agree
| * @param opts - Options to pass to the Redis client. | ||
| */ | ||
| constructor( | ||
| opts?: RedisOptions |
There was a problem hiding this comment.
Maybe Redis instance must be passed as a constructor parameter?
| async loadRange(keyPrefix: string[]): Promise<Chunk[]> { | ||
| const lowerBound = [...keyPrefix, '*'].join(":") | ||
| const result: Chunk[] = [] | ||
| const stream = this.redis.scanStream({ |
|
@pvh hello, Your phrase: "though then we lose automated test integration... hmm" reminded me the Promises/A+ Compliance Test Suite. What do you think of the idea of developing a set of tests that could be used to test any implementation of the |
|
Sounds great if it can be kept simple. I think the tricky part here is operational complexity -- for things like nodefs & indexeddb there are easily available implementations but for Redis, Postgres, or other backends, we'll need to have the ability to spin up the service at runtime. For SaaS-only backends (say, S3) there's yet more complexity. That said, I'm quite interested in a solution. I think @HerbCaudill was the one who designed the system we use to test the network adapters and as we grow beyond a monorepo, I think something along the lines you propose could be valuable. |
|
Yeah, i think it would be a good idea to restructure the storage adapter tests that currently live in StorageSubsystem.test.ts so that they work more like the network adapter acceptance tests here - you can look in each of the existing network adapters' tests to see how they're invoked. |
|
@kid-icarus hi, The test suit for a storage adapter implementation merged recently #307 and ready to be used! |
|
Awesome, thank you @bijela-gora! I'll take a look sometime in the next week or so 🙏🏻 |
|
@kid-icarus How do you feel about hosting this as a standalone package at /automerge/automerge-repo-storage-redis/? I think my intuition is that adapters which rely on things not found in most browsers / OS should have their own repo and then whatever CI we set up can be independent. |
|
I'd like to share that I created an SQLite storage adapter for JS/TS projects: https://github.com/bijela-gora/automerge-repo-storage-better-sqlite3 I created it because I'm going to use it in my pet project. Thank you for the awesome Automerge project! |
That makes sense from a principled perspective 👍🏻 One pro of a separate repo is more control over CI, being able to throw whatever external dependencies into a Dockerfile, etc. Another pro is that if there are breaking changes in automerge(-repo), namely the types in this specific case, the burden isn't placed on the core team to update the adapter in the monorepo. One con would be extra care to specify automerge and automerge-repo as a combination of dev and peer dependencies, so that adapter consumers aren't using duplicate, potentially incompatible versions of them. This also means that as newer versions of automerge(-repo) are released, we'll need to bump those dev dependencies. Dependabot can help with that, but it's just the general con of something living in a monorepo vs an external repo. That said, I'm more than happy to have the package live in a separate repo :) |
pvh
force-pushed
the
main
branch
2 times, most recently
from
e61f8e3
to
d3d1a7d
Compare
Adding this for review. I've tested this locally and it seems to work just fine. Ultimately I'd like to add some integration tests here, as it has none, similar to the nodefs storage adapter.
For that to work, I was considering leveraging a test container to ensure that Redis is installed, but that would require an update to our test github action (to install docker).
I meant to open this PR earlier but thought I'd get a conversation started :)