feat: preserve base when constructed from a string #77

olizilla · 2019-04-03T09:36:45Z

BREAKING CHANGE - previously base was not preserved and all CIDs would
be normalised to base58btc when asked for their string representation.

The default will change to base32 in https://github.com/multiformats/js-cid/pull/73/files

The idea behind this change is that we shouldnt lose information when
the user passes us a base encoded string, but keep it and use it as
the default base so toString returns the same string they provided.

I'd like this as a fix for ipld explorer, which currently forces all
CIDs into base58btc, seee: ipfs/ipfs-webui#999

This PR is the smallest change I can see to make it work. Do we want
to also add base as a constructor parameter? If so, i guess it should
go at the end as it's optional, but thats aesthetically displeasing
as the base encoding goes at the front in string form.

fixes #76

License: MIT
Signed-off-by: Oli Evans oli@tableflip.io

vmx

@olizilla You can see my confusion about base and this.base. If you think that code is perfect and can't be made clearer and I should just spent time learning JavaScript, let me know and I'll do that.

src/index.js

vmx · 2019-04-03T12:06:37Z

src/index.js

+    /**
+     * @type {string}
+     */
+    this.base = base || 'base58btc'


Is

this.base = base || 'base58btc' ------------^^^^ this

the let base from the constructor? If yes, would a comment help to clarify that? Perhaps at the let base definition, to make clear that it's intentionally there and not just by mistake.

Yes, that base is the from the let base. The confusing thing here is the existing logic in the constructor... the first case checks if what was passed was a CID, copies the values across directly... e.g this.version = cid.version and returns early. The reminaing checks mutate the value of the arguments and eventually assign them to the this instance being set up the constructor. Neither is good or bad, but having both styles in one constructor is weird.

olizilla · 2019-04-03T13:25:25Z

I kept this PR small as this module could use a refactor, but I wanted to focus on the essence of the change first, but you are right, this code is harder to follow than it needs to be. I'll take another pass.

olizilla · 2019-04-03T14:18:45Z

@vmx I have a treat for you. I've clarified things, relied on spooky scopes less, and made it so each check in the constructor sets all the things on the new instance rather than maybe mutating the arguments, and return early from each one, so the state you have to keep in your head to figure out what its doing is shorter lived.

The tests pass locally for me, so I've no idea what travis is up to. Looking into it now.

olizilla · 2019-04-03T14:21:34Z

Oh commitlint! Subject must not end with a full stop!!! wtf. i. like. sentences.

Please can we kill commitlint for PRs... Its on the maintainer to squash and merge a PR branch with a commit message they are happy with. Getting a red x for a full stop in a commit message is demoralising, and I don't think it adds enough value to be worth that.

vmx

Thanks a lot, this makes things clearer.

BTW: The earlier version was also clearer than I thought, I misread things due to seeing the indentation level wrong.

vmx · 2019-04-03T14:22:28Z

src/index.js

+        this.version = 0
+        this.codec = 'dag-pb'
+        this.multihash = mh.fromB58String(version)
+        this.multibaseName = multibaseName


This is correct and only a matter of taste, so feel free to keep it like it is. I would hard-code it to base58btc to make clear that v0 can't have another base.

vmx · 2019-04-03T14:23:09Z

src/index.js

+        this.version = 0
+        this.codec = 'dag-pb'
+        this.multihash = version
+        this.multibaseName = multibaseName


Same as the comment above, I'd hard-code base58btc.

vmx · 2019-04-03T14:25:13Z

src/index.js

@@ -196,9 +220,7 @@ class CID {
   * @param {string} [base='base58btc'] - Base encoding to use.


That's not correct anymore, the base is now the one given my the constructor.

olizilla · 2019-04-03T14:46:07Z

Open questions:

Are two CIDs equal if they point to the same multihash but have a differenent default multibaseName for their string representation. I'm assuming yes.
Should toV1 preserve an existing multibaseName if it's not the default? I've assumed no, as it's more useful it toV1 gives you a CID in the most common format.

vmx · 2019-04-03T15:03:36Z

Are two CIDs equal if they point to the same multihash but have a differenent default multibaseName for their string representation. I'm assuming yes.

Yes!

Should toV1 preserve an existing multibaseName if it's not the default? I've assumed no, as it's more useful it toV1 gives you a CID in the most common format.

I'm unsure, on one hand I'd say "no" as the most common case seems to be converting from V0 to V1. On the other hand I might expect a call to toV1() on a CID that is already V1 to be a no-op.

alanshaw · 2019-04-03T15:31:56Z

Please can we kill commitlint for PRs...

Yes please! ipfs/aegir#345

alanshaw

This looks great, in the future we could cache a string CID so that toString can just return it when no multibase is specified without having to re-encode each time.

Do we need to test you can't create an invalid v0 CID?:

new CID(0, 'dag-pb', Buffer.from(/*...*/), 'base32')
new CID(0, 'dag-cbor', Buffer.from(/*...*/), 'base58btc') (existing issue I think!)

vmx

Thanks a lot @olizilla. I think it's ready to be merged. If you're OK with it I'd force push a squahed version.

BREAKING CHANGE: previously base was not preserved and all CIDs would be normalised to base58btc when asking for their string representation. The default will change to base32 in https://github.com/multiformats/js-cid/pull/73/files The idea behind this change is that we shouldnt lose information when the user passes us a base encoded string, but keep it and use it as the default base so toString returns the same string they provided. I'd like this as a fix for ipld explorer, which currently forces all CIDs into base58btc, seee: ipfs/ipfs-webui#999 License: MIT Signed-off-by: Oli Evans <oli@tableflip.io>

Adds tests for - base other than 'base32btc' for CIDv0 - codec other than 'dag-pb' for CIDv0 License: MIT Signed-off-by: Oli Evans <oli@tableflip.io>

olizilla · 2019-04-04T10:59:32Z

I got carried away and added the string caching that @alanshaw suggested. Sorry @vmx would you mind having another look.

Please feel free to squash and merge this to master as you see fit. I always hit the Squash and merge option in github and groom the commit message.

store the string form of the CID as a non-enumerable property where an instance is constructed from a valid string, or after the first call to toBaseEncodedString where the supplied base matches the default base. adds tests for string caching, and some missing test cases. License: MIT Signed-off-by: Oli Evans <oli@tableflip.io>

vmx · 2019-04-04T11:07:54Z

@olizilla I've edited the commit message a bit. I sadly can't a request a review from you, but feel free to thumb up this comment if you think it's ready to go (once the CI passed).

olizilla · 2019-04-04T11:30:22Z

@vmx I can't see what changed, but I think this PR is ready to go!

As per the commit-lint discussion, I tend to leave contributors PR branch commits as they provided them (feels nice, you see the progression, and they can refer back to it) and then use the Squash and merge feature to collapse it down to a single commit to master, where I get to groom the commit message for the change log.

vmx · 2019-04-04T11:36:01Z

@olizilla I just changed the commit message and nothing of the contents. What I started to do is having individual commits piled up and in the end to a squash locally as sometime i want to preserve some commits and force push that. This way I have CI running (and commit linting) and can be sure everything is fine.

vmx · 2019-04-04T11:37:29Z

@olizilla I just saw that the README isn't up to date about preserving the CID, would you mind updating it?

olizilla · 2019-04-04T14:03:03Z

Will do!

Add 4th optional parameter introduced by multiformats#77

Add 4th optional parameter introduced by #77

- update to cids@0.6.0 for multiformats/js-cid#77 fixes: #999 License: MIT Signed-off-by: Oli Evans <oli@tableflip.io>

ghost assigned olizilla Apr 3, 2019

ghost added the status/in-progress In progress label Apr 3, 2019

olizilla requested a review from vmx April 3, 2019 09:37

vmx reviewed Apr 3, 2019

View reviewed changes

olizilla marked this pull request as ready for review April 3, 2019 14:47

olizilla requested a review from alanshaw April 3, 2019 15:10

alanshaw approved these changes Apr 3, 2019

View reviewed changes

hugomrdias approved these changes Apr 3, 2019

View reviewed changes

vmx approved these changes Apr 4, 2019

View reviewed changes

olizilla added 2 commits April 4, 2019 12:05

chore: validate and test CIDv0 invariants

350f58c

Adds tests for - base other than 'base32btc' for CIDv0 - codec other than 'dag-pb' for CIDv0 License: MIT Signed-off-by: Oli Evans <oli@tableflip.io>

alanshaw approved these changes Apr 4, 2019

View reviewed changes

vmx force-pushed the preserve-base branch from f08e50e to a5d28bd Compare April 4, 2019 11:05

ghost assigned vmx Apr 4, 2019

vmx merged commit 537f604 into master Apr 4, 2019

ghost removed the status/in-progress In progress label Apr 4, 2019

vmx deleted the preserve-base branch April 4, 2019 11:36

This was referenced Apr 8, 2019

feat: add flow typedefs #79

Merged

CID formatting changes ipfs/ipfs-webui#999

Closed

Gozala added a commit to Gozala/js-cid that referenced this pull request Apr 8, 2019

fix: update typedefs to reflect API changes

9ed3a13

Add 4th optional parameter introduced by multiformats#77

Gozala mentioned this pull request Apr 8, 2019

fix: update typedefs to reflect API changes #81

Merged

vmx pushed a commit that referenced this pull request Apr 8, 2019

fix: update typedefs to reflect API changes

63cd5f3

Add 4th optional parameter introduced by #77

olizilla added a commit to ipfs/ipfs-webui that referenced this pull request Apr 9, 2019

fix: preserve user provided cid base

800b77b

- update to cids@0.6.0 for multiformats/js-cid#77 fixes: #999 License: MIT Signed-off-by: Oli Evans <oli@tableflip.io>

This was referenced Apr 9, 2019

fix: preserve user provided cid base ipfs/ipfs-webui#1005

Merged

Provide non-generic constructor methods #83

Open

olizilla added a commit to ipfs/ipfs-webui that referenced this pull request Apr 10, 2019

fix: preserve user provided cid base (#1005)

bde4c69

- update to cids@0.6.0 for multiformats/js-cid#77 fixes: #999 License: MIT Signed-off-by: Oli Evans <oli@tableflip.io>

vasco-santos mentioned this pull request Jun 4, 2019

IPNS Pubsub tests failing on master for linux ipfs/interop#71

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: preserve base when constructed from a string #77

feat: preserve base when constructed from a string #77

olizilla commented Apr 3, 2019 •

edited

Loading

vmx left a comment

vmx Apr 3, 2019

olizilla Apr 3, 2019

olizilla commented Apr 3, 2019

olizilla commented Apr 3, 2019

olizilla commented Apr 3, 2019 •

edited

Loading

vmx left a comment

vmx Apr 3, 2019

olizilla Apr 3, 2019

vmx Apr 3, 2019

vmx Apr 3, 2019

olizilla commented Apr 3, 2019

vmx commented Apr 3, 2019

alanshaw commented Apr 3, 2019

alanshaw left a comment

vmx left a comment

olizilla commented Apr 4, 2019

vmx commented Apr 4, 2019

olizilla commented Apr 4, 2019

vmx commented Apr 4, 2019

vmx commented Apr 4, 2019

olizilla commented Apr 4, 2019

		@@ -196,9 +220,7 @@ class CID {
		* @param {string} [base='base58btc'] - Base encoding to use.

feat: preserve base when constructed from a string #77

feat: preserve base when constructed from a string #77

Conversation

olizilla commented Apr 3, 2019 • edited Loading

vmx left a comment

Choose a reason for hiding this comment

vmx Apr 3, 2019

Choose a reason for hiding this comment

olizilla Apr 3, 2019

Choose a reason for hiding this comment

olizilla commented Apr 3, 2019

olizilla commented Apr 3, 2019

olizilla commented Apr 3, 2019 • edited Loading

vmx left a comment

Choose a reason for hiding this comment

vmx Apr 3, 2019

Choose a reason for hiding this comment

olizilla Apr 3, 2019

Choose a reason for hiding this comment

vmx Apr 3, 2019

Choose a reason for hiding this comment

vmx Apr 3, 2019

Choose a reason for hiding this comment

olizilla commented Apr 3, 2019

vmx commented Apr 3, 2019

alanshaw commented Apr 3, 2019

alanshaw left a comment

Choose a reason for hiding this comment

vmx left a comment

Choose a reason for hiding this comment

olizilla commented Apr 4, 2019

vmx commented Apr 4, 2019

olizilla commented Apr 4, 2019

vmx commented Apr 4, 2019

vmx commented Apr 4, 2019

olizilla commented Apr 4, 2019

olizilla commented Apr 3, 2019 •

edited

Loading

olizilla commented Apr 3, 2019 •

edited

Loading