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

inMemory collections don't implement static methods #744

Closed
rafamel opened this issue Aug 8, 2018 · 11 comments
Closed

inMemory collections don't implement static methods #744

rafamel opened this issue Aug 8, 2018 · 11 comments
Labels

Comments

@rafamel
Copy link
Contributor

rafamel commented Aug 8, 2018

Case

Not sure on whether it is a bug or just a feature lack.

Issue

inMemory collections don't implement static methods available for the parent collection.

Info

  • Environment: (Node.js/browser/electron/etc..)
  • Adapter: (IndexedDB/Localstorage/LevelDB/etc..)
  • Stack: Babel, React
  • RxDB: 7.7.1

Code

const RxDB = require('rxdb');
RxDB.plugin(require('pouchdb-adapter-idb'));
RxDB.plugin(require('pouchdb-adapter-memory'));

async function test() {
  const db = await RxDB.create({
    name: 'testdb',
    adapter: 'idb',
    multiInstance: false
  });

  const schema = {
    version: 0,
    type: 'object',
    primaryPath: '_id',
    properties: {
      name: {
        type: 'string'
      }
    }
  };

  const collection = await db.collection({
    name: 'person',
    schema: schema,
    statics: {
      one() {
        return this.findOne();
      }
    }
  });

  const inMemCollection = await db.collections.person.inMemory();

  collection.insert({ name: 'hi' });

  try {
    const one = await inMemCollection.one().exec();
    console.log('inMemory collection:', one.name);
  } catch (e) {
    console.log('inMemory collection fail', e);
  }

  const oneIdb = await collection.one().exec();
  console.log('idb collection:', oneIdb.name);
}
test();
@pubkey
Copy link
Owner

pubkey commented Aug 8, 2018

Thats a feature lack. Thanks for reporting. Im on holidays for a few weeks so this will take some time until I fix it.

@rafamel
Copy link
Contributor Author

rafamel commented Aug 10, 2018

Related to this, they also don't inherit the options property of the parent. Just leaving it here for reference

@rafamel
Copy link
Contributor Author

rafamel commented Aug 19, 2018

Hi Daniel. I have invested quite a bit of time playing with RxDB these past days. It seems to me like, now that we have QueryChangeDetector, the point of having inMemory collections is almost null, while the burden of maintaining both is probably higher. QueryChangeDetector seems like a more elegant solution to the problem of improving the response times to me. Additionally, there are a few issues with the implementation of inMemory. There's this, #754 , and the fact that they won't be accessible by other collections via the usual this.database.collections on collections, and this.collection.database.collections on documents. They also won't be used to improve the response time of relations populate() queries.

For those reasons, I would suggest to remove inMemory collections altogether. It's true that the response time for the first result of queries won't be improved by QueryChangeDetector, but I think that's a reasonable trade-off to make. And, ultimately, the same functionality could be manually achieved by creating a memory rxdb instance and registering + syncing the desired collections, while giving the developers a more fine-grained control on how it all fits together. I would rather want to see a recipe for doing this being provided than keep inMemory as part of the RxDB core.

I would close the issue, but not sure if you'll agree; let me know what you think!

@pubkey pubkey closed this as completed in b558b02 Aug 19, 2018
@pubkey
Copy link
Owner

pubkey commented Aug 19, 2018

The things that are optimiseable by the query-change-detection are limited. Depending on what type of queries you do, it might be possible that next to every query runs against the hard-drive.
For most UI-things, the query-change-detection might be enough, but in my tests, it made sense to use the inMemory-Collection for some type of data.
Since the inMemory-functionality as a plugin, you can use rxdb without it and create an own logic for in-memory synchronisation.

I fixed the original issue, I will think about your arguments a bit more but close this issue for now.

@rafamel
Copy link
Contributor Author

rafamel commented Aug 20, 2018

That makes total sense. If you feel like you could share it, it'd be great to have the insight those tests gave you so we'd know specifically which use cases suit them best.

In any case, if inMemory collections are kept in RxDB, I'd like to suggest a different way of handling these. As things are right now, we're creating a new type of collections that are not accessible on the collections object of the DB and we are responsible to pass around to use and handle separately. They also have a different lifecycle than their equivalents, and are not used internally for population.

Instead of duplicating all the code and having a different type of collections that are "in memory", I would propose that collection.inMemory() doesn't return a brand new inMemory collection instance, but instead creates a in memory pouchdb instance, sets it as its source of data (collection.pouch) and syncs it with the previous/original one. So essentially, we have the same collection, with the same lifecycle, with its data linked to an in memory pouch instance, which is in turn synced with an underlying pouch instance for data persistance. This would essentially solve most current problems with in memory collections and, most importantly, make dealing with them way more convenient. Additionally, there could be another collection method to remove it from memory and return it to its original state - just in case we were worried about this.

The only downside I can see is we wouldn't have two separate instances, which I personally would view as an upside as it makes dealing with them more straightforwards, and I can't see a use case for the opposite. And even if there are extreme use cases, there's probably a case to be made for simplifying the inMemory api, which will benefit most people using it, in exchange for dropping support for these - which would nevertheless be achievable with a little bit of more code on the user part.

Let me know what you think!
Rafa

@pubkey
Copy link
Owner

pubkey commented Aug 20, 2018

I can not share my tests, but it is basically about handling data for time-tracking. For example if i want to display all entries that are younger than one hour, I would write a query like find().where('time').lt(new Date().getTime()) which will not be optimised because getTime() produces a different query each time.

I see a problem with changing the internal pouchdb:
Creating the memory-puchdb and replication takes tame and so it must be async. Therefore when you call collection.inMemory() and at the same time another part of your app does some doc-operation on the collection, you would not know if the operation is written into the memory-pouch or the one before.
Also if the app crashes at that time, you would not know for sure if the write was persistent because if that is a requirement, you must always write to the non-memory-version.

Another thing is, that it was planned to allow adding a RxQuery when you call inMemory() to create the memory-collection from only a subset of the whole data. Mutation the original collection would disallow to add this feature in the future.

I will try to fix all issues with the current inMemory-implementation in the next days and maybe I have a different opinion then :)

@rafamel
Copy link
Contributor Author

rafamel commented Aug 21, 2018

You're absolutely right on your point. One solution I can think of is for the create/update/delete queries no to resolve until they're written on both, while all queries run again the memory one. But even then it wouldn't allow for your planned implementation of allowing subsets of the collection to be in memory, which I can see a few use cases for.

I think it'd be good to stick with your original plan forwards. However, following your appreciation in that "you would not know if the operation is written into the memory-pouch or the one before", that's also the case for in memory collections as they are - when we write to them, which we'll inevitably do if we recover docs from it through any query and want to write on the docs. There might be a case to be made for replacing all create/update/delete ops so they operate on the non in memory pouch db underlying instance, while reads occur in memory.

Also, are they inheriting hooks too after the rewrite?

@pubkey
Copy link
Owner

pubkey commented Aug 21, 2018

This is a great idea. We should try doing writes directly to the parent-collection and only do queries at the in-memory. I will add this to the road of 8.0.0 since it changes the behavior and maybe break things that worked before.

No I have not touched the hooks.

@pubkey pubkey reopened this Aug 21, 2018
@pubkey pubkey added the 8.0.0 label Aug 21, 2018
@rafamel
Copy link
Contributor Author

rafamel commented Aug 22, 2018

Great!

Regarding hooks, as writes would go to the permanent storage pouchdb instance, I'm thinking it would make sense to apply the same hooks as the parent collection does. What do you think?

Edit: I was reflecting some more on the idea of not creating a separate instance but realized what I was proposing would be easily done with a plugin, as your proposed solution would be more flexible.

@pubkey
Copy link
Owner

pubkey commented Aug 28, 2018

@rafamel I changed the in-memory-plugin so writes now go directly to the parent collection and it is awaited until the write was replicated.

I do not think we should copy the middleware-hooks because they are not set when the collection is created, but afterwards by calling myCollection.preInsert() for example. This means that the hook is not a immutable state of the collection but a changeable value, so you could reset the hook by calling preInsert again. The reason for not settings hooks on collection-creation, was to be similar to mongoosejs which maybe was not a good path.

Also I am not sure what should happen when the hook is copied and I run an insert on a document. Because we do the write to the parent collection, does that mean that the hook runs twice? With the current implementation, only the preInsert-hook of the inMemory-collection would run.

Maybe you see a use-case where copying the hook is a better solution?

@pubkey
Copy link
Owner

pubkey commented Aug 28, 2018

I am closing this because the original issue is resolved.
If there are more arguments to change the hooks-behavior, please open a new issue.

@pubkey pubkey closed this as completed Aug 28, 2018
rafamel added a commit to rafamel/rxdb that referenced this issue Sep 14, 2018
rafamel added a commit to rafamel/rxdb that referenced this issue Sep 14, 2018
@rafamel rafamel mentioned this issue Sep 14, 2018
pubkey pushed a commit that referenced this issue Sep 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants