BREAKING CHANGE: Remove .cid, .multihash and .serialized properties #99

achingbrain · 2018-10-31T12:34:58Z

Follows on from ipld/js-ipld#173 (comment)

These properties are removed from the DAGNode class.

.multihash is removed because they aren't multihashes any more
.cid is removed to bring dag-pb in line with other ipld types
.serialized is removed because storing data buffers and the serialized form uses too much memory - we can use the utils.serialize method to create the serialized form when we need it, which in this module is just during the tests

.multihash has also changed to .cid in the output of DAGLink.toJSON and DAGNode.toJSON because since CIDv1 they are not just multihashes any more; the multihash is contained within the CID.

vmx · 2018-10-31T12:54:10Z

First quick comment (as gatekeeper of good/correct commit messages). This won't pass commit lint. According to our style it would be:

feat: remove .cid, .multihash and .serialized properties

Follows on from https://github.com/ipld/js-ipld/issues/173#issuecomment-434408680

BREAKING CHANGE:
These properties are removed from the DAGNode class.

* `.multihash` is removed because they aren't multihashes any more
* `.cid` is removed to bring dag-pb in line with other ipld types
* `.serialized` is removed because storing data buffers and the
  serialized form uses too much memory - we can use the utils.serialize
  method to create the serialized form when we need it, which in this
  module is just during the tests

`.multihash` has also changed to `.cid` in the output of
`DAGLink.toJSON` and `DAGNode.toJSON` because since CIDv1 they are
not just multihashes any more; the multihash is contained within
the CID.

achingbrain · 2018-10-31T13:15:53Z

It would be nice if we could discover these things before CI gets involved. I'd added commit linting to aegir, but you can only lint commits once they've been committed (I think), so it had to be part of a pre-push hook, which we removed from all of our repos a little while back. 🤷‍♂️

vmx · 2018-10-31T13:32:26Z

This repo (and all other IPLD repos) still have a pre-push hook :)

achingbrain · 2018-10-31T13:45:19Z

Cool, well you can add aegir lint-commits to them and never be surprised by CI ever again. Well, not for commit messages anyway.

achingbrain · 2018-10-31T13:45:27Z

We are way off topic.

vmx · 2018-10-31T15:45:33Z

@achingbrain Is it OK if I wait for a merge until the new js-ipfs release is out? I think this change has a lot of implications (probably just as many as the dag-cbor change had).

README.md

achingbrain · 2018-10-31T16:14:45Z

@vmx yes, please wait and do other releases in the meantime - if for nothing else other than I'm getting PRs ready for unixfs and mfs that will fix the breakage this will cause.

codecov · 2018-10-31T17:00:05Z

Codecov Report

Merging #99 into master will increase coverage by 1.06%.
The diff coverage is 90.9%.

@@            Coverage Diff             @@
##           master      #99      +/-   ##
==========================================
+ Coverage   90.28%   91.35%   +1.06%     
==========================================
  Files          13       13              
  Lines         278      266      -12     
==========================================
- Hits          251      243       -8     
+ Misses         27       23       -4

Impacted Files	Coverage Δ
src/dag-link/util.js	`100% <ø> (ø)`	⬆️
src/dag-link/create.js	`50% <0%> (ø)`	⬆️
src/dag-node/index.js	`88% <100%> (+3.15%)`	⬆️
src/dag-link/index.js	`90% <100%> (+3.63%)`	⬆️
src/util.js	`97.91% <100%> (+3.79%)`	⬆️
src/dag-node/create.js	`87.5% <100%> (-1.08%)`	⬇️
src/resolver.js	`94.2% <100%> (ø)`	⬆️
src/dag-node/rmLink.js	`92.85% <83.33%> (+0.54%)`	⬆️
src/dag-node/addLink.js	`90.9% <84.61%> (-3.54%)`	⬇️
src/dag-node/util.js	`91.3% <87.5%> (-2.45%)`	⬇️

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 dafad86...42e0700. Read the comment docs.

achingbrain · 2018-11-06T15:03:48Z

How about removing the size property too? It's the size of the serialized buffer that a DAGNode would be turned into by util.serialize and doesn't seem to be the sort of thing that a data object would know about. It also means we don't have to calculate the serialized size on creation so we'd save a bunch of CPU cycles there too.

achingbrain · 2018-11-06T16:55:04Z

This does make calculating the return values from ipfs.files.add difficult as it contains the byte size of the DAGNode tree that stores a given file (e.g. including protobuf wrappers, serialized DAGLinks, etc).

Is that information actually useful to anyone though? I remember my surprise when the size returned from ipfs.files.add was not the size of the file I'd passed in.

achingbrain · 2018-11-07T08:11:07Z

I'm going to back out the removal of .size for the time being until we've got some numbers on it being a bottleneck as it has ramifications for the API.

achingbrain · 2018-11-07T08:37:48Z

I've opened ipfs-inactive/interface-js-ipfs-core#387 to talk about the size property returned from ipfs.files.add a bit more.

daviddias · 2018-11-07T11:13:15Z

@achingbrain ack on ipfs-inactive/interface-js-ipfs-core#387. Do you plan to block this PR on that issue? I recommend that we make that change as separate.

achingbrain · 2018-11-07T11:41:06Z

@diasdavid - I agree, that issue should not block this PR, we can make the change later once we've got consensus on whether to make the API change or not.

Follows on from ipld/js-ipld-dag-pb#99 and updates this module to not rely on DAGNodes having knowledge of their CIDs.

Follows on from ipld/js-ipld-dag-pb#99 and updates this module to not rely on DAGNodes having knowledge of their CIDs. Bonus: also removes use of js-ipfs from this module breaking another circular dependency from the project.

Follows on from ipld/js-ipld-dag-pb#99 and updates this module to not rely on DAGNodes having knowledge of their CIDs. Bonus: also fixes #24 by removing use of js-ipfs from this module breaking another circular dependency from the project. License: MIT Signed-off-by: achingbrain <alex@achingbrain.net>

…operties BREAKING CHANGE: DAGNodes no longer have `.cid` or `.multihash` properties - see ipld/js-ipld-dag-pb#99 for more and and #173 for the performance win. Follows on from ipld/js-ipld-dag-pb#99 and updates this module to not rely on DAGNodes having knowledge of their CIDs. Fixes the tests which could use a closer look at, for example they overwrite the value of `node3` twice.

BREAKING CHANGE: DAGNodes no longer have `.cid` or `.multihash` properties - see ipld/js-ipld-dag-pb#99 for more and and #173 for the performance win. Follows on from ipld/js-ipld-dag-pb#99 and updates this module to not rely on DAGNodes having knowledge of their CIDs. Fixes the tests which could use a closer look at, for example they overwrite the value of `node3` twice.

Follows on from ipld/js-ipld-dag-pb#99 and updates this module to not rely on DAGNodes having knowledge of their CIDs. Bonus: also removes use of js-ipfs from this module breaking another circular dependency from the project.

Follows on from ipld/js-ipld-dag-pb#99 and updates this module to not rely on DAGNodes having knowledge of their CIDs. Bonus: also fixes #24 by removing use of js-ipfs from this module breaking another circular dependency from the project. License: MIT Signed-off-by: achingbrain <alex@achingbrain.net>

Follows on from ipld/js-ipld-dag-pb#99 and updates this module to not rely on DAGNodes having knowledge of their CIDs.

Just having a slight wobble about removing the `.multihash` property from the DAGLink class as part of #99. It's the right thing to do but it will cause a lot of disruption.

Follows on from ipld/js-ipld-dag-pb#99 and updates this module to not rely on DAGNodes having knowledge of their CIDs.

BREAKING CHANGE: Object API refactor. Object API methods that write DAG nodes now return a CID instead of a DAG node. Affected methods: * `ipfs.object.new` * `ipfs.object.patch.addLink` * `ipfs.object.patch.appendData` * `ipfs.object.patch.rmLink` * `ipfs.object.patch.setData` * `ipfs.object.put` Example: ```js // Before const dagNode = await ipfs.object.new() ``` ```js // After const cid = await ipfs.object.new() // now returns a CID const dagNode = await ipfs.object.get(cid) // fetch the DAG node that was created ``` IMPORTANT: `DAGNode` instances, which are part of the IPLD dag-pb format have been refactored. These instances no longer have `multihash`, `cid` or `serialized` properties. This effects the following API methods that return these types of objects: * `ipfs.object.get` * `ipfs.dag.get` See ipld/js-ipld-dag-pb#99 for more information. License: MIT Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>

For the back story on this change, please see: ipfs-inactive/interface-js-ipfs-core#388 (review) BREAKING CHANGE: Object API refactor. Object API methods that write DAG nodes now return a [CID](https://www.npmjs.com/package/cids) instead of a DAG node. Affected methods: * `ipfs.object.new` * `ipfs.object.patch.addLink` * `ipfs.object.patch.appendData` * `ipfs.object.patch.rmLink` * `ipfs.object.patch.setData` * `ipfs.object.put` Example: ```js // Before const dagNode = await ipfs.object.new() ``` ```js // After const cid = await ipfs.object.new() // now returns a CID const dagNode = await ipfs.object.get(cid) // fetch the DAG node that was created ``` IMPORTANT: `DAGNode` instances, which are part of the IPLD dag-pb format have been refactored. These instances no longer have `multihash`, `cid` or `serialized` properties. This effects the following API methods that return these types of objects: * `ipfs.object.get` * `ipfs.dag.get` See ipld/js-ipld-dag-pb#99 for more information. License: MIT Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>

BREAKING CHANGE: Object API refactor. Object API methods that write DAG nodes now return a CID instead of a DAG node. Affected methods: * `ipfs.object.new` * `ipfs.object.patch.addLink` * `ipfs.object.patch.appendData` * `ipfs.object.patch.rmLink` * `ipfs.object.patch.setData` * `ipfs.object.put` Example: ```js // Before const dagNode = await ipfs.object.new() ``` ```js // After const cid = await ipfs.object.new() // now returns a CID const dagNode = await ipfs.object.get(cid) // fetch the DAG node that was created ``` IMPORTANT: `DAGNode` instances, which are part of the IPLD dag-pb format have been refactored. These instances no longer have `multihash`, `cid` or `serialized` properties. This effects the following API methods that return these types of objects: * `ipfs.object.get` * `ipfs.dag.get` See ipld/js-ipld-dag-pb#99 for more information. License: MIT Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>

For the back story on this change, please see: ipfs-inactive/interface-js-ipfs-core#388 (review) BREAKING CHANGE: Object API refactor. Object API methods that write DAG nodes now return a [CID](https://www.npmjs.com/package/cids) instead of a DAG node. Affected methods: * `ipfs.object.new` * `ipfs.object.patch.addLink` * `ipfs.object.patch.appendData` * `ipfs.object.patch.rmLink` * `ipfs.object.patch.setData` * `ipfs.object.put` Example: ```js // Before const dagNode = await ipfs.object.new() ``` ```js // After const cid = await ipfs.object.new() // now returns a CID const dagNode = await ipfs.object.get(cid) // fetch the DAG node that was created ``` IMPORTANT: `DAGNode` instances, which are part of the IPLD dag-pb format have been refactored. These instances no longer have `multihash`, `cid` or `serialized` properties. This effects the following API methods that return these types of objects: * `ipfs.object.get` * `ipfs.dag.get` See ipld/js-ipld-dag-pb#99 for more information. License: MIT Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>

Follows on from ipld/js-ipld-dag-pb#99 and updates this module to not rely on DAGNodes having knowledge of their CIDs.

ghost assigned achingbrain Oct 31, 2018

ghost added the status/in-progress In progress label Oct 31, 2018

achingbrain requested review from vmx and daviddias October 31, 2018 12:52

achingbrain force-pushed the remove-cid-property-from-dagnodes branch from 2d85ba9 to 64a0f21 Compare October 31, 2018 12:56

vmx suggested changes Oct 31, 2018

View reviewed changes

README.md Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

achingbrain force-pushed the remove-cid-property-from-dagnodes branch from 40fea4f to 2e901bb Compare November 7, 2018 08:10

achingbrain mentioned this pull request Nov 7, 2018

fix: updates ipld-dag-pb dep to version without .cid properties ipfs-inactive/js-ipfs-unixfs-engine#233

Merged

achingbrain mentioned this pull request Nov 8, 2018

fix: updates ipld-dag-pb dep to version without .cid or .multihash properties ipfs-inactive/js-ipfs-mfs#25

Merged

achingbrain mentioned this pull request Nov 12, 2018

fix: reinstate DAGLink.multihash property with deprecation notice #102

Closed

achingbrain added a commit to ipfs/js-ipfs that referenced this pull request Nov 13, 2018

fix: updates ipld-dag-pb dep to version without .cid properties

fda2bec

Follows on from ipld/js-ipld-dag-pb#99 and updates this module to not rely on DAGNodes having knowledge of their CIDs.

achingbrain added a commit to ipfs/js-ipfs that referenced this pull request Nov 13, 2018

fix: updates ipld-dag-pb dep to version without .cid properties

b0eab93

Follows on from ipld/js-ipld-dag-pb#99 and updates this module to not rely on DAGNodes having knowledge of their CIDs.

alanshaw mentioned this pull request Nov 19, 2018

⚡️ v0.34.0 RELEASE 🚀 ipfs/js-ipfs#1721

Closed

49 tasks

alanshaw pushed a commit to ipfs/js-ipfs that referenced this pull request Nov 20, 2018

fix: updates ipld-dag-pb dep to version without .cid properties

4f0c0e5

Follows on from ipld/js-ipld-dag-pb#99 and updates this module to not rely on DAGNodes having knowledge of their CIDs.

achingbrain added a commit to ipfs/js-ipfs that referenced this pull request Nov 25, 2018

fix: updates ipld-dag-pb dep to version without .cid properties

9e1bace

Follows on from ipld/js-ipld-dag-pb#99 and updates this module to not rely on DAGNodes having knowledge of their CIDs.

achingbrain added a commit to ipfs/js-ipfs that referenced this pull request Nov 26, 2018

fix: updates ipld-dag-pb dep to version without .cid properties

2e1579e

Follows on from ipld/js-ipld-dag-pb#99 and updates this module to not rely on DAGNodes having knowledge of their CIDs.

achingbrain added a commit to ipfs/js-ipfs that referenced this pull request Nov 26, 2018

fix: updates ipld-dag-pb dep to version without .cid properties

5cdd89b

Follows on from ipld/js-ipld-dag-pb#99 and updates this module to not rely on DAGNodes having knowledge of their CIDs.

alanshaw mentioned this pull request Nov 27, 2018

refactor: object API write methods now return CIDs ipfs-inactive/js-ipfs-http-client#896

Merged

alanshaw mentioned this pull request Nov 27, 2018

refactor: object APIs write methods now return CIDs ipfs/js-ipfs#1730

Merged

achingbrain mentioned this pull request Dec 17, 2018

Pass CID into util.deserialize? ipld/js-ipld#173

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BREAKING CHANGE: Remove .cid, .multihash and .serialized properties #99

BREAKING CHANGE: Remove .cid, .multihash and .serialized properties #99

achingbrain commented Oct 31, 2018

vmx commented Oct 31, 2018

achingbrain commented Oct 31, 2018

vmx commented Oct 31, 2018

achingbrain commented Oct 31, 2018

achingbrain commented Oct 31, 2018

vmx commented Oct 31, 2018

achingbrain commented Oct 31, 2018

codecov bot commented Oct 31, 2018 •

edited

Loading

achingbrain commented Nov 6, 2018

achingbrain commented Nov 6, 2018

achingbrain commented Nov 7, 2018 •

edited

Loading

achingbrain commented Nov 7, 2018

daviddias commented Nov 7, 2018

achingbrain commented Nov 7, 2018

BREAKING CHANGE: Remove .cid, .multihash and .serialized properties #99

BREAKING CHANGE: Remove .cid, .multihash and .serialized properties #99

Conversation

achingbrain commented Oct 31, 2018

vmx commented Oct 31, 2018

achingbrain commented Oct 31, 2018

vmx commented Oct 31, 2018

achingbrain commented Oct 31, 2018

achingbrain commented Oct 31, 2018

vmx commented Oct 31, 2018

achingbrain commented Oct 31, 2018

codecov bot commented Oct 31, 2018 • edited Loading

Codecov Report

achingbrain commented Nov 6, 2018

achingbrain commented Nov 6, 2018

achingbrain commented Nov 7, 2018 • edited Loading

achingbrain commented Nov 7, 2018

daviddias commented Nov 7, 2018

achingbrain commented Nov 7, 2018

codecov bot commented Oct 31, 2018 •

edited

Loading

achingbrain commented Nov 7, 2018 •

edited

Loading