bajankristof / nedb-promises

A dead-simple promise wrapper for nedb.
MIT License
298 stars 45 forks source link

Embed nedb and rewrite it #46

Closed tex0l closed 3 years ago

tex0l commented 3 years ago

Following the discussion on https://github.com/bajankristof/nedb-promises/issues/45, I feel like this is a good time to fork nedb for good, cleanup everything, and expose a native Promise-based interface instead of shimming it.

This PR currently:

What still needs to be done is:

tex0l commented 3 years ago

could use nodes built in mkdir instead

Yes, good idea, done in https://github.com/bajankristof/nedb-promises/pull/46/commits/4bc10f07089cba265edc2253ff07be131c330e4d

tex0l commented 3 years ago

change stuff to classes?

Yes, this is indeed a good idea, I really want to move step by step to avoid breaking anything.

EDIT: done in https://github.com/bajankristof/nedb-promises/pull/46/commits/7e703cd2bd9a3a9ec961ef4faf88ff8f5b49bde4

bajankristof commented 3 years ago

I would also rename some of the files in the original nedb and binarySearchTree packages to better match the contents of this library (for example avltree.js -> AVLTree.js, bst.js -> BinarySearchTree.js)

It looks like only 2 functions of underscore are used (omit, uniq), @jimmywarting already proposed a solution to replace omit, so here is my proposal for uniq (with arity 1, 2):

function uniqueArray(array, projector) {
    return array.reduce((acc, value, index, source) => {
        if (source.indexOf(value) !== index) {
            return acc;
        }

        projected = projector ? projector(value) : value;
        acc.push(projected);
        return acc;
    }, []);
}

Since this functionality is only used in nedb it could be placed in nedb/customUtils.js.

I will continue to look into the code and the PR when I have time.

jimmywarting commented 3 years ago

Another way to handle uniqueness is by using Set

// spread out the Set to a new array - even better would be to keep it as a Set
uniqArr = [...new Set(arr)]

But i'm not sure if it would do the same thing...

bajankristof commented 3 years ago

Another way to handle uniqueness is by using Set

// spread out the Set to a new array - even better would be to keep it as a Set
uniqArr = [...new Set(arr)]

But i'm not sure if it would do the same thing...

That is probably good as well, for uniq with 2 args we would need to map the elements using the projector function beforehand though:

[...new Set(array.map(projectUniqueKeys))]

It is definetly more readable and clear this way.

tex0l commented 3 years ago

Ok, after I gave it some thought, I adopted another strategy: we will release a fork of nedb which will be backward-compatible with 1.8.0, and this fork will do just the same as what I proposed in this PR, including your comments.

We'll eventually expose a Promise-based interface. Meanwhile, you'll be able to switch to @seald-io/nedb@^2.0.0 in nedb-promises as soon as I have finished cleaning it up (you can expect it today or tomorrow).

This is still a work in progress, but here it is:

tex0l commented 3 years ago

Ok @jimmywarting @bajankristof I released the v2.0.0 of our fork, it has been tested internally against our tests suites along the original test suite : it works as a drop-in replacement. I believe you can simply base nedb-promises on this fork, I'll maintain it with my team : https://github.com/seald/nedb

As for the rest of the modernization, here is the roadmap: https://github.com/seald/nedb#modernization, don't hesitate to make a PR :).