Implement LRU Reference Delegate for Memory Persistence #1237
Conversation
gsoltis
requested review from
mikelehen,
rsgowman,
var-const,
wilhuff and
zxu123
as code owners
|
|
||
| private _started = false; | ||
|
|
||
| private _referenceDelegate: ReferenceDelegate | null; | ||
| get referenceDelegate(): ReferenceDelegate { |
There was a problem hiding this comment.
If you do keep this, please move it down past the constructor.
There was a problem hiding this comment.
As above, this will eventually just be a property. It's just waiting on the final reference delegate implementation.
There was a problem hiding this comment.
I would still like to push you to move this down to where we define other methods.
|
|
||
| private _started = false; | ||
|
|
||
| private _referenceDelegate: ReferenceDelegate | null; |
There was a problem hiding this comment.
We have been using just ReferenceDelegate as a the type if a member is essentially always set. Would it be possible to pass the delegate into the constructor from your factory function? You could then also make the variable itself public and remove the getter.
There was a problem hiding this comment.
Can I punt on this until the next PR? That one will introduce MemoryEagerDelegate, and then there will always be a reference delegate for memory persistence.
There was a problem hiding this comment.
Please add a TODO to clean this up then :)
| promises.push(referenceDelegate.addReference(txn, key)); | ||
| }); | ||
| } | ||
| return PersistencePromise.waitFor(promises); |
There was a problem hiding this comment.
If the set of keys here is small, you could also do:
return PersistencePromise.forEach(keys.toArray(), key -> referenceDelegate.addReference(txn, key));
There was a problem hiding this comment.
I don't know if it's small? It's however many keys are added to a target.
This PR adds only the lru reference delegate for memory. The merge base is into
lru-master.Android Port: FirebasePrivate/firebase-android-sdk#190
iOS Port: firebase/firebase-ios-sdk#1600