Options as destructured parameters (and sortFn for static functions, while we're at it)

aphelionz commented 5 years ago

Proposal

Migrate from this:

static async fromCID (ipfs, access, identity, cid, length = -1, exclude, onProgressCallback)

to this:

static async fromCID (ipfs, access, identity, cid, { length = -1, exclude, onProgressCallback, sortFn })

For the following functions

[x] Log constructor
[x] Log.fromJSON
[x] Log.fromEntry
[x] Log.fromCID
[x] Log.fromMultihash
[x] Log.fromEntryCID
[x] Log.fromEntryHash
[x] LogIO.fromCID
[x] LogIO.fromEntry
[x] LogIO.fromEntryCID
[x] EntryIO.fetchParallel
[x] EntryIO.fetchAll

This is a nice middle ground between signature transparency and a more convenient developer experience.

Resolves #219

Other changes (e.g. bug fixes, UI tweaks, refactors)

I also added the sortFn argument to the destructured options, since it was quick and easy to do. Need tests.

TODO

[x] Complete refactor of all functions in the description above
[x] Figure out what to do with standardjs /home/mark/Projects/orbitdb/ipfs-log/src/log.js:467:5: Return statement should not contain assignment.
[x] Commit build output files

TODO: Docstring Updates

[x] Log constructor
[x] Log.fromJSON
[x] Log.fromEntry
[x] Log.fromCID
[x] Log.fromMultihash
[x] Log.fromEntryCID
[x] Log.fromEntryHash
[x] LogIO.fromCID
[x] LogIO.fromEntry
[x] LogIO.fromEntryCID
[x] EntryIO.fetchParallel
[x] EntryIO.fetchAll

TODO: Tests

[x] Log.fromEntry
[x] Log.fromCID
[x] Log.fromMultihash
[x] Log.fromJSON
[x] Log.fromEntryCID
[x] Log.fromEntryHash

aphelionz commented 5 years ago

cc @satazor

satazor commented 5 years ago

@aphelionz

Shouldn't the Log constructor also be changed to have options instead of all the optional arguments it has now? I would also look for other functions that have the same issue and change as well.
If we want to make this backwards-compatible, we should create a helper that would take all the optional arguments and their order and normalize it to an options object. This way, we could reuse it everywhere and maintain compatibility.

static async fromCID (ipfs, access, identity, cid, ...rest) {
    const { length, exclude, onProgressCallback } = normalizeOptions(rest, ['length', 'exclude', 'onProgressCallback'])
}

aphelionz commented 5 years ago

@satazor Yes, I thought of that and others - such as the Entry constructor that have similar signatures. However, I'll admit I'm just nervous about making such a big change.

normalizeOptions seems like a good idea as I love the idea of not having to change the tests and break the API. So, that seems pretty good.

@shamb0t I would love another set of eyes on this as well if you have a second.

aphelionz commented 5 years ago

@satazor The matter of default values comes into question with the normalizeOptions approach. Currently we have, for example, length: -1. I'm not sure how to do that effectively here. perhaps an object merge after normalizeOptions is called?

shamb0t commented 5 years ago

This looks good so far @aphelionz! It would be preferable to change all function signatures to match this approach. Agreed that we can have a defaultOptions object and just merge them with Object.assign({}, defaultOptions, normalizedOptions) after normalizing. We should also add access to the options as there is a default value. Re: normalize helper function, I think it may be preferable to break the API (given the next release will do that anyways) I like the idea of a "normalizeOptions' method that can sanitize and ensure correct shape but I dont think backwards compatibility is necessary here so no need to remember the order of arguments passed.

satazor commented 5 years ago

@aphelionz yes we would have to receive the default options and mixin:

const { length, exclude, onProgressCallback } = normalizeOptions(rest, { length: -1, exclude: [] }, ['length', 'exclude', 'onProgressCallback'])

You may change the order of arguments in normalizeOptions as you see fit.

aphelionz commented 5 years ago

If we're breaking the API, then it's just simpler to keep it as the destructured arguments in the function signature instead of the spread syntax.

So, keep:

static async fromCID (ipfs, access, identity, cid, { length = -1, exclude, onProgressCallback, sortFn })

instead of:

static async fromCID (ipfs, access, identity, cid, ...options)

This handles defaults, is explicit in what it expects, and doesn't require the normalize helper function.

If we agree, then all I need is a list of functions to give this treatment to (Log constructor, Entry constructor, etc) and then I can move forward and get this ready for merge.

shamb0t commented 5 years ago

@aphelionz I agree that way is simpler.

Looks to me like the functions that need treatment are the constructor in Log, static async fromJSON, static async fromEntry, static async fromEntryCid, static async fromMultihash in LogIO and static async fetchParallel and static async fetchAll in EntryIO.

satazor commented 5 years ago

If we're breaking the API, then it's just simpler to keep it as the destructured arguments in the function signature instead of the spread syntax.

I agree, if we are doing a breaking change then keep it simple. If we want to maintain compatibility, create the normalizeOptions function.

aphelionz commented 5 years ago

Whew! Got through the functionality. Just to write tests and docstring. Please review.

satazor commented 5 years ago

@aphelionz This looks good to me!

aphelionz commented 5 years ago

Getting a strange error on build. It looks familiar but I forget how we solved it in the past.

ERROR in ./node_modules/datastore-level/node_modules/level-js/iterator.js
Module not found: Error: Can't resolve 'idb-readable-stream' in '/home/mark/Projects/orbitdb/ipfs-log/node_modules/datastore-level/node_modules/level-js'
 @ ./node_modules/datastore-level/node_modules/level-js/iterator.js 4:24-54
 @ ./node_modules/datastore-level/node_modules/level-js/index.js
 @ ./node_modules/datastore-level/src/index.js
 @ ./node_modules/ipfs/node_modules/ipfs-repo/src/default-options-browser.js
 @ ./node_modules/ipfs/node_modules/ipfs-repo/src/index.js
 @ ./node_modules/ipfs/src/core/boot.js
 @ ./node_modules/ipfs/src/core/index.js
 @ ./examples/browser/index.js

cc @shamb0t @satazor @haadcode

aphelionz commented 5 years ago

Fixed the above error

aphelionz commented 5 years ago

Here's another interesting finding.

There we were no tests written for fromJSON so I added one. This uncovered a bug, a log.length discrepancy where it would return a different value than expected.

This comes from fetchParallelit, which traces all traversal paths from heads concurrently. For example, one traversal path would be A1 - A10, while another would be A1, B1, A2, B2, A3, etc... This is expected and good. However, but when it merges them it does not filter out duplicates. In the case of this test, 21 entries were returned instead of the expected 16.

You can see here where I augmented the concatArrays function to be the uniquelyConcat arrays function.

aphelionz commented 5 years ago

@satazor @shamb0t @haadcode This is ready for a comprehensive review and potential merge.

aphelionz commented 5 years ago

@haadcode @shamb0t @satazor Shall we merge?

shamb0t commented 5 years ago

@aphelionz LGTM :+1:

orbitdb-archive / ipfs-log