-
Notifications
You must be signed in to change notification settings - Fork 37
refactor: pass in IPLD Formats into the constructor #164
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,73 +14,26 @@ const map = require('async/map') | |
const series = require('async/series') | ||
const waterfall = require('async/waterfall') | ||
const MemoryStore = require('interface-datastore').MemoryDatastore | ||
const mergeOptions = require('merge-options') | ||
const ipldDagCbor = require('ipld-dag-cbor') | ||
const ipldDagPb = require('ipld-dag-pb') | ||
const ipldRaw = require('ipld-raw') | ||
|
||
function noop () {} | ||
|
||
class IPLDResolver { | ||
constructor (options) { | ||
constructor (userOptions) { | ||
const options = mergeOptions(IPLDResolver.defaultOptions, userOptions) | ||
|
||
if (!options.blockService) { | ||
throw new Error('Missing blockservice') | ||
} | ||
|
||
this.bs = options.blockService | ||
|
||
// Support by default dag-pb, dag-cbor, git, and eth-* | ||
this.resolvers = { | ||
get 'dag-pb' () { | ||
const format = require('ipld-dag-pb') | ||
return { resolver: format.resolver, util: format.util } | ||
}, | ||
get 'dag-cbor' () { | ||
const format = require('ipld-dag-cbor') | ||
return { resolver: format.resolver, util: format.util } | ||
}, | ||
get 'git-raw' () { | ||
const format = require('ipld-git') | ||
return { resolver: format.resolver, util: format.util } | ||
}, | ||
get 'bitcoin-block' () { | ||
const format = require('ipld-bitcoin') | ||
return { resolver: format.resolver, util: format.util } | ||
}, | ||
get 'eth-account-snapshot' () { | ||
const format = require('ipld-ethereum').ethAccountSnapshot | ||
return { resolver: format.resolver, util: format.util } | ||
}, | ||
get 'eth-block' () { | ||
const format = require('ipld-ethereum').ethBlock | ||
return { resolver: format.resolver, util: format.util } | ||
}, | ||
get 'eth-block-list' () { | ||
const format = require('ipld-ethereum').ethBlockList | ||
return { resolver: format.resolver, util: format.util } | ||
}, | ||
get 'eth-state-trie' () { | ||
const format = require('ipld-ethereum').ethStateTrie | ||
return { resolver: format.resolver, util: format.util } | ||
}, | ||
get 'eth-storage-trie' () { | ||
const format = require('ipld-ethereum').ethStorageTrie | ||
return { resolver: format.resolver, util: format.util } | ||
}, | ||
get 'eth-tx' () { | ||
const format = require('ipld-ethereum').ethTx | ||
return { resolver: format.resolver, util: format.util } | ||
}, | ||
get 'eth-tx-trie' () { | ||
const format = require('ipld-ethereum').ethTxTrie | ||
return { resolver: format.resolver, util: format.util } | ||
}, | ||
get raw () { | ||
const format = require('ipld-raw') | ||
return { resolver: format.resolver, util: format.util } | ||
}, | ||
get 'zcash-block' () { | ||
const format = require('ipld-zcash') | ||
return { resolver: format.resolver, util: format.util } | ||
} | ||
} | ||
// Object with current list of active resolvers | ||
this.resolvers = {} | ||
|
||
// API entry point | ||
this.support = {} | ||
|
||
// Adds support for an IPLD format | ||
|
@@ -100,6 +53,13 @@ class IPLDResolver { | |
delete this.resolvers[multicodec] | ||
} | ||
} | ||
|
||
// Enable all supplied formats | ||
for (const format of options.formats) { | ||
const {resolver, util} = format | ||
const multicodec = resolver.multicodec | ||
this.support.add(multicodec, resolver, util) | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This'll undo the perf work here #145 as IPFS (and potentially others) will have to require these formats upfront. Can we load these on demand? This would actually be good for IPFS in browsers because we could exclude the implementations from the bundle but add loaders to load in the code when/if needed. Something like this?: // In IPFS:
ipld.support.add('git-raw', () => new Promise((resolve, reject) => {
// Could replace with a XHR request. In Node.js we just:
resolve(require('ipld-git'))
})) // In ipld
let format
if (typeof this.support[codec] === 'function') {
format = await this.support[codec]()
this.support[codec] = format
} else if (this.support[codec]) {
format = this.support[codec]
} else {
throw new Error('missing format')
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My idea was that you'd normally pass in only the things you need anyway. But then I found out that this is certainly not the case for the js-ifps CLI, where you want support for all formats. That's a good idea. Thanks for also flushing out the code. |
||
} | ||
|
||
get (cid, path, options, callback) { | ||
|
@@ -414,6 +374,13 @@ class IPLDResolver { | |
} | ||
} | ||
|
||
/** | ||
* Default options for IPLD. | ||
*/ | ||
IPLDResolver.defaultOptions = { | ||
formats: [ipldDagCbor, ipldDagPb, ipldRaw] | ||
} | ||
|
||
/** | ||
* Create an IPLD resolver with an inmemory blockservice and | ||
* repo. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be enough here right ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want a to have a deep copy. Example:
Output is:
See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/assign#Deep_Clone for more information about
Object.assign()
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok just wanted to make sure we really need deep copy here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
K, cool. I prefer code which doesn't lead to weird surprises in the long run (+ I've coded functional programming language for a looong tim)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vmx can i suggest https://www.npmjs.com/package/assign-deep instead of merge-options. Check https://codesandbox.io/s/742p0rjvy0, assign-deep is smaller and handles non-plain-objects values better like promises (merge-options gives empty object when you should get a Promise). We should use assign-deep everywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hugomrdias: I want to have a deep copy, that makes things easier to debug/reason about.
assign-deep
does only a shallow copy.Though I'd love to see that we agree on something decent across all libs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just read ipfs/js-ipfs#1663 (comment). Perhaps it's not always a good idea to copy things. I think I need to put more thought into this. Though I'll merge this for now to keep things going.