louischatriot / nedb

The JavaScript Database, for Node.js, nw.js, electron and the browser
MIT License
13.49k stars 1.03k forks source link

Persistence in the browser #124

Closed goloroden closed 10 years ago

goloroden commented 10 years ago

In the readme you wrote that you'll

[...] implement persistence later.

I don't want to put you in a hurry, but, do you have already any plans on when later will be?

louischatriot commented 10 years ago

Hello,

Very good question indeed :) I don't have a lot of time for that currently but it should not be too much work. I may do it within the next few weeks but the best way would b e for you to open a PR if you need it quickly. I'll happoly review it.

Cheers, Louis

louischatriot commented 10 years ago

PS: note to self, I need to reply to this tweet once it's done: https://twitter.com/rauschma/status/424725060981903360

negue commented 10 years ago

Hey,

here I have an example for how to use the localStorage as Persistence for NeDB:

https://gist.github.com/negue/6be16350db328458d379

I don*t know if this code is correct, but it seemed to work for me:

self.db.indexes._id.insert(data);

This Example isn't optimized yet, and maybe even slow for bigger datasets.

Edit: Now everything should work (updates, deletes, indexes) with the Code of #122

marshallswain commented 10 years ago

@negue As I was browsing through the code, I noticed that sublime linter was "upset" about the __proto__ property. Since this is the first time that I've seen that property, I did a quick search to learn about it and found this page stating that it's going away. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/proto Not sure if you already knew about it, but FYI.

negue commented 10 years ago

Yeah this line var Index = tempDB.indexes._id.__proto__.constructor; was just there since my test Persistence was not in the normal JS File, if its landing to the File inside "browser-version/browser-specific/lib" , then there wouldn't be any need for this "proto" trick^^

marshallswain commented 10 years ago

I see. I've been testing it out and it seems to work great. The only issue I had with it was accidentally wiping out data by trying to run some of the other functions instead of loadDatabase(), but I don't think that will be a problem once it's merged in.

marshallswain commented 10 years ago

@negue How did you get updates and deletes to work? I'm using code from 122 like you mentioned (just using nedb.min.js), but any time I do an update or a delete, it ends up as though it has done an insert.

marshallswain commented 10 years ago

@negue Nevermind. I misunderstood how it was supposed to work. I read through the server version's persistence.js file and now see that the data file grows larger with the instructions, then you run compactDataFile to compress it back down, removing the extra commands ($$delete and record rewrites for updates). So , it appears to be working perfectly. Sorry for the trouble. :)

marshallswain commented 10 years ago

Now I see why this trumps TaffyDB's speed. Very well engineered.

skotchio commented 10 years ago

Hey guys! Thanks for the work on this feature. May you tell me when do you plan to push this one into the master? I really want to realize it in my project.

marshallswain commented 10 years ago

@vovan22 It seems that we ought to submit a PR with a cleaned up version of @negue's code that includes tests to show it's working. That way @louischatriot could simply read the code, run the tests, and merge the request. He's obviously busy, as am I. I'll see if I can get around to making a PR this weekend to move it forward.

skotchio commented 10 years ago

@marshallswain thanks. I'm very excited this feature.

skotchio commented 10 years ago

Hey guys. Tell me how's it going around this issue? Does anybody complete this one?

negue commented 10 years ago

@marshallswain What kind of tests are needed for this persistence?

Maybe I'll create them and make a proper pullrequest then.

marshallswain commented 10 years ago

The tests would each need to

  1. Modify the database
  2. Use localStorage API to check that the appropriate changes were appended to the string.

Probably a test for each of these would be more than enough:

I think that would be enough since everything else kinda depends on those three.

Sorry. It's too easy to get busy.

goloroden commented 10 years ago

What needs to be done with this issue before the pull request can be merged in?

louischatriot commented 10 years ago

I actually coded browser persistence myself and didn't accept the PR, this one was quite tricky to get exactly right. NeDB browser now supports persistence as of v0.11 : https://github.com/louischatriot/nedb#browser-version

fabiobon commented 10 years ago

I Louis, I've tried nedb#browser-version with localStorage persistence. After any first different document property update, document are duplicated instead of been replaced. Is it normal? for example: db.insert({ _id:'2', planet: 'Mars' }); db.update({ _id: '2' }, { $set: { "data.satellites": 2, "data.red": true } }, {}, function () {}); db.update({ _id: '2' }, { $set: { "data.satellites": 3, } }, {}, function () {});

db.find({ planet: 'Mars' }); returns one document, but inside of localStorage value there are three documents.

Given the limits of the size of localStorage, I think it's better to replace the document rather than duplicate it. What do you think about this?

Thanx for your job!

fabiobon commented 10 years ago

Sorry I've not read the documentation: yourDatabase.persistence.compactDatafile

louischatriot commented 10 years ago

Yeah, the documents are not dupplicates but differnet versions, and only the latest is taken into account. Your point is interesting though: given the limits on localStorage size, making a lot of updates and deletes without reloading the page (when a compaction occurs) might cause problems. Autocompaction is useful in that case, I'll add a note in the readme about that