Skip to content
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

Service keeps all records in memory #116

Open
DaddyWarbucks opened this issue May 21, 2022 · 1 comment · May be fixed by #117
Open

Service keeps all records in memory #116

DaddyWarbucks opened this issue May 21, 2022 · 1 comment · May be fixed by #117

Comments

@DaddyWarbucks
Copy link
Contributor

Expected behavior

The storage item is only retrieved when needed. Meaning that the entire collection is not in memory except when needed.

Actual behavior

On the first service call, the entire storage collection is loaded into memory. The collection is then not read from storage again.

I totally understand why current implementation works the way it does, because its an easy extension of the feathers-memory service. But I would also like to see an implementation that does not keep the collection in memory when not needed. I am using localstorage services to backup rest/socket services. But, I don't want all of that data to persist in memory when not being used which is 99% of the time. And when it is being used, I don't mind paying the penalty of having to retrieve it from storage each time.

But, I don't currently see how this can be done without major changes to the current implementation, mainly the fact that service.store is expected to be persistent and developer may be using it in their own code.

I do have one idea, which is to set the store to null when flushing. But that would break expected behavior of service.store always being available (or at least after the first service call, which is weird DX also)

 flush (data) {
   if (!this._timeout) {
     this._timeout = setTimeout(() => {
       this._storage.setItem(this._storageKey, JSON.stringify(this.store));
       delete this._timeout;
       
       // Reset the store, releasing it from memory
       if (this._strict) {
         this.store = null;
       }
     }, this._throttle);
   }

   return data;
 }
 
 // Also flush after reads

find (...args) {
   return this.execute('find', ...args)
     .then(data => this._strict ? this.flush(data) : data);
 }

 get (...args) {
   return this.execute('get', ...args)
     .then(data => this._strict ? this.flush(data) : data);
 }
 
 // other methods already flush

This is untested, but is pretty simple and slick. It does break the guarantee that service.store stays defined, but that is already a bit weird because service.store is not defined until after the first service call anyway. So breaking that guarantee (behind the options.strict flag) doesn't seem that bad.

I am happy to open a PR to add this and some tests, docs etc. But, this is a bit of

@daffl
Copy link
Member

daffl commented May 21, 2022

Disregard my delete comment 😆 exactly what you said should work.

@DaddyWarbucks DaddyWarbucks linked a pull request May 21, 2022 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants