-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: updated DataStore [2/n] #141
Conversation
Now largely complete, will be best to review once all items in #139 are done, because this might still change. |
981070a
to
4da0146
Compare
f316efc
to
1c8ff87
Compare
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.
couple of minor suggestions. also need to remove a test.solo
call to see if all the added tests pass, but otherwise seems good to me
Is the plan to eventually use this as the datastore implementation (currently named to avoid collision with existing one)
if (['newListener', 'removeListener'].includes(eventName)) return | ||
this.#coreIndexer.on(eventName, listener) | ||
}) | ||
this.on('removeListener', (eventName, listener) => { | ||
if (['newListener', 'removeListener'].includes(eventName)) return |
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.
extract these events to a constant? i.e. const LISTENER_EVENTS = ['newListener', 'removeListener']
(you can come up with a better variable name 😄 )
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'd prefer not to because I'd like this to be explicit in the code, rather than the indirection of a constant - we'd end up naming it "newListenerAndRemoveListenerEventNames". Making is a constant might be a slight performance improvement, but this is not somewhere that is a bottleneck (events are added rarely).
Co-authored-by: Andrew Chou <[email protected]>
2111be7
to
370fbb9
Compare
Rebased onto changes in #140 |
Merged in #149 |
From #139:
A
DataStore
should be an abstraction over a particular "Mapeo Store", e.g. a collection of hypercores that store a particular datatype. Responsibilities:With this we can isolate the boundary with hypercore / core store to this module (apart from within the blob store), and largely isolate indexing: The write method can resolve when the document is indexed.