Skip to content
This repository has been archived by the owner on Aug 11, 2021. It is now read-only.

refactor: pass in IPLD Formats into the constructor #164

Merged
merged 1 commit into from
Oct 25, 2018
Merged

Conversation

vmx
Copy link
Member

@vmx vmx commented Oct 18, 2018

Instead of having IPLD using all currently available IPLD Format
implementations automatically, pass the Formats that should be
used into the constructor as options.

This will also decrease the bundle size as every user of IPLD can
decide which IPLD Formats should be used. By default
ipld-dag-cbor and ipld-dag-pb are used as those are the ones
also heavily used within the IPFS ecosystem.

BREAKING CHANGE: Not all IPLD Formats are included by default

By default only the ipld-dag-cbor and ipld-dag-pb IPLD Formats
are included. If you want to use other IPLD Formats you need
to pass them into the constructor.

The code to restore the old behaviour could look like this:

const ipldDagPb = require('ipld-dag-pb')
const ipldDagCbor = require('ipld-dag-cbor')
const ipldGit = require('ipld-git')
const ipldBitcoin = require('ipld-bitcoin')
const ipldEthAccountSnapshot = require('ipld-ethereum').ethAccountSnapshot
const ipldEthBlock = require('ipld-ethereum').ethBlock
const ipldEthBlockList = require('ipld-ethereum').ethBlockList
const ipldEthStateTrie = require('ipld-ethereum').ethStateTrie
const ipldEthStorageTrie = require('ipld-ethereum').ethStorageTrie
const ipldEthTx = require('ipld-ethereum').ethTx
const ipldEthtrie = require('ipld-ethereum').ethTxTrie
const ipldRaw = require('ipld-raw')
const ipldZcash = require('ipld-zcash')



const ipld = new Ipld({
  blockService: blockService,
  formats: [
    pldDagPb, ipldDagCbor, ipldGit, ipldBitcoin, ipldEthAccountSnapshot,
    ipldEthBlock, ipldEthBlockList, ipldEthStateTrie, ipldEthStorageTrie,
    ipldEthTx, ipldEthtrie, ipldRaw, ipldZcash
  ]
})

/cc @alanshaw @hugomrdias

@vmx vmx requested a review from daviddias October 18, 2018 16:53
@vmx
Copy link
Member Author

vmx commented Oct 19, 2018

It's not clear, but the parts of the CI that matter passed (and the failed part can't be retriggered).


function noop () {}

class IPLDResolver {
constructor (options) {
constructor (userOptions) {
const options = mergeOptions(IPLDResolver.defaultOptions, userOptions)
Copy link
Member

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 ?

Suggested change
const options = mergeOptions(IPLDResolver.defaultOptions, userOptions)
const options = Object.assign({}, IPLDResolver.defaultOptions, userOptions)

Copy link
Member Author

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:

'use strict'

const mergeOptions = require('merge-options')

const oa1 = {main: {sub: 1}}
const oa2 = {main: {sub: 2}}
const oa = Object.assign({}, oa1, oa2)
oa2.main.sub = 3
console.log('oa:', oa)

const mo1 = {main: {sub: 1}}
const mo2 = {main: {sub: 2}}
const mo = mergeOptions(mo1, mo2)
mo2.main.sub = 3
console.log('mo:',  mo)

Output is:

oa: { main: { sub: 3 } }
mo: { main: { sub: 2 } }

See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/assign#Deep_Clone for more information about Object.assign().

Copy link
Member

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.

Copy link
Member Author

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)

Copy link
Member

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.

Copy link
Member Author

@vmx vmx Oct 25, 2018

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.

Copy link
Member Author

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.

Copy link
Member

@hugomrdias hugomrdias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@daviddias daviddias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These PR should be done in tandem to PR to IPLD examples in js-ipfs and also add an example to this repo that shows how to add the multple formats.

@vmx
Copy link
Member Author

vmx commented Oct 22, 2018

@diasdavid I've done the js-ipfs change already locally. Once this change is merged and released. I will make PRs to those repos updating js-ipld with the corresponding changes. There's not point of keep too many PRs open for a long time.

@daviddias
Copy link
Member

@vmx we've been working and encouraging on working with Open PRs since the beginning of the project. It helps track work and get others to see and experiment as well. When there is a PR that requires many other PRs, then we link then and make sure that we explain that on the top level PR.

@vmx vmx force-pushed the options-api branch 2 times, most recently from 5546e70 to ea2bba9 Compare October 23, 2018 14:39
@ipld ipld deleted a comment from codecov bot Oct 23, 2018
@codecov
Copy link

codecov bot commented Oct 23, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@f1c2e12). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #164   +/-   ##
=========================================
  Coverage          ?   89.13%           
=========================================
  Files             ?        1           
  Lines             ?      184           
  Branches          ?        0           
=========================================
  Hits              ?      164           
  Misses            ?       20           
  Partials          ?        0
Impacted Files Coverage Δ
src/index.js 89.13% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f1c2e12...669ee0c. Read the comment docs.

Instead of having IPLD using all currently available [IPLD Format]
implementations automatically, pass the Formats that should be
used into the constructor as options.

This will also decrease the bundle size as every user of IPLD can
decide which IPLD Formats should be used. By default
[ipld-dag-cbor] and [ipld-dag-pb] are used as those are the ones
also heavily used within the IPFS ecosystem.

BREAKING CHANGE: Not all IPLD Formats are included by default

By default only the [ipld-dag-cbor], [ipld-dag-pb] and [raw]
[IPLD Format]s are included. If you want to use other IPLD Formats
you need to pass them into the constructor.

The code to restore the old behaviour could look like this:

```js
const ipldBitcoin = require('ipld-bitcoin')
const ipldDagCbor = require('ipld-dag-cbor')
const ipldDagPb = require('ipld-dag-pb')
const ipldEthAccountSnapshot = require('ipld-ethereum').ethAccountSnapshot
const ipldEthBlock = require('ipld-ethereum').ethBlock
const ipldEthBlockList = require('ipld-ethereum').ethBlockList
const ipldEthStateTrie = require('ipld-ethereum').ethStateTrie
const ipldEthStorageTrie = require('ipld-ethereum').ethStorageTrie
const ipldEthTrie = require('ipld-ethereum').ethTxTrie
const ipldEthTx = require('ipld-ethereum').ethTx
const ipldGit = require('ipld-git')
const ipldRaw = require('ipld-raw')
const ipldZcash = require('ipld-zcash')

…

const ipld = new Ipld({
  blockService: blockService,
  formats: [
    ipldBitcoin, ipldDagCbor, ipldDagPb, ipldEthAccountSnapshot,
    ipldEthBlock, ipldEthBlockList, ipldEthStateTrie, ipldEthStorageTrie,
    ipldEthTrie, ipldEthTx, ipldGit, ipldRaw, ipldZcash
  ]
})
```

[ipld-dag-cbor]: https://github.com/ipld/js-ipld-dag-cbor
[ipld-dag-pb]: https://github.com/ipld/js-ipld-dag-pb
[ipld-raw]: https://github.com/ipld/js-ipld-raw
[IPLD Format]: https://github.com/ipld/interface-ipld-format
@vmx
Copy link
Member Author

vmx commented Oct 24, 2018

@diasdavid Projects I've identified that need changes if this change is released:

There is already an example on how to add other formats in the README.

@vmx vmx merged commit b003ad1 into master Oct 25, 2018
@vmx vmx deleted the options-api branch October 25, 2018 10:12
const {resolver, util} = format
const multicodec = resolver.multicodec
this.support.add(multicodec, resolver, util)
}
Copy link
Member

Choose a reason for hiding this comment

The 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')
}

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

alanshaw pushed a commit that referenced this pull request Nov 8, 2018
This backwards compatible PR allows missing IPLD formats to be loaded dynamically.

This follows on from the discussion here #164 (comment)

A new constructor option `loadFormat` allows users to asynchronously load an IPLD format. The IPLD instance will call this function automatically when it is requested to resolve a format that it doesn't currently understand.

This can re-enable lazy loading for IPLD modules in IPFS and also means that people who just want to add _more_ resolvers can do so without having to depend directly on the defaults and pass them in as well.

License: MIT
Signed-off-by: Alan Shaw <[email protected]>
vmx pushed a commit that referenced this pull request Nov 9, 2018
This backwards compatible PR allows missing IPLD formats to be loaded dynamically.

This follows on from the discussion here #164 (comment)

A new constructor option `loadFormat` allows users to asynchronously load an IPLD format. The IPLD instance will call this function automatically when it is requested to resolve a format that it doesn't currently understand.

This can re-enable lazy loading for IPLD modules in IPFS and also means that people who just want to add _more_ resolvers can do so without having to depend directly on the defaults and pass them in as well.

License: MIT
Signed-off-by: Alan Shaw <[email protected]>
alanshaw pushed a commit to ipfs/js-ipfs that referenced this pull request Nov 9, 2018
This PR uses the new `loadFormat` option for IPLD to lazily require IPLD formats in order to reduce the startup time for the node.

If you're feeling like you've seen this before then, for reference:

The PR ipld/js-ipld#164 undid the work done in ipld/js-ipld#145 and ipld/js-ipld#178 re-enabled lazy loading.

License: MIT
Signed-off-by: Alan Shaw <[email protected]>
alanshaw pushed a commit to ipfs/js-ipfs that referenced this pull request Nov 12, 2018
This PR uses the new `loadFormat` option for IPLD to lazily require IPLD formats in order to reduce the startup time for the node.

If you're feeling like you've seen this before then, for reference:

The PR ipld/js-ipld#164 undid the work done in ipld/js-ipld#145 and ipld/js-ipld#178 re-enabled the ability to do so. This PR makes use of this new ability to lazy load the formats.

License: MIT
Signed-off-by: Alan Shaw <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants