Incompatibility with "zii"

[KyleMaas]

Describe the bug
If the package "zii" is installed due to a dependency somewhere else in an npm project including js-multibase (in my case, from ipfs-repo-migrations from ipfs-core), this function:

js-multibase/src/index.js

Line 116 in c373031

function encoding (nameOrCode) {

...no longer works for types which use a code of "z". This happens in my case from here:

https://github.com/ipfs/js-ipfs-repo-migrations/blob/425d53c5f2ef2d89b11d2cfc33362d5ebfad81a5/migrations/migration-9/utils.js#L10

To Reproduce
Steps to reproduce the behavior:

1. npm install js-multibase
2. npm install zii
3. Require both so they are pulled into your project.
4. Try to decode "zQmdfTbBqBPQ7VNxZEYEj14VmRuZBkqFbiwReogJgS1zR1n"

Expected behavior
It decodes.

Actual behavior
I get this error: TypeError: enc.decode is not a function

Desktop (please complete the following information):

• OS: Gentoo Linux
• Browser: Chromium
• Version: 87

[KyleMaas]

I should note that this is where I'm trying to use this:

arj03/ssb-browser-demo#198

zii is pulled in a couple levels of dependencies down, so it's not an easy one for us to just patch out.

[jacobheun]

Where did you get the CID "zQmdfTbBqBPQ7VNxZEYEj14VmRuZBkqFbiwReogJgS1zR1n"? It's not valid. I think there may be some bad generation of the hashes in one of the dependencies that's incorrectly appending 'z' instead of properly processing it.

[KyleMaas]

That comes from this:

https://github.com/ipfs/js-ipfs-repo-migrations/blob/425d53c5f2ef2d89b11d2cfc33362d5ebfad81a5/migrations/migration-9/utils.js#L10

Which calls this:

https://github.com/multiformats/js-multihash/blob/e93d3a25c164d16c0c02d0747c68381f62d77930/src/index.js#L72

Which prepends a "z" and then tries to decode.

However, the problem happens before it even gets to the point where it would be valid or not. It comes from autodetection of encoding of hashes which start with a "z" trigger. If they start with the letter "z", this is true if zii is active:

js-multibase/src/index.js

Line 117 in c373031

if (constants.names[/** @type {BaseName} */(nameOrCode)]) {

So it returns from here:

js-multibase/src/index.js

Line 118 in c373031

return constants.names[/** @type {BaseName} */(nameOrCode)]

Instead of falling back to:

js-multibase/src/index.js

Line 119 in c373031

} else if (constants.codes[/** @type {BaseCode} */(nameOrCode)]) {

And returning from:

js-multibase/src/index.js

Line 120 in c373031

return constants.codes[/** @type {BaseCode} */(nameOrCode)]

It triggers the zii override gets because there is nothing in constants.names with the key "z". But because zii adds a prototype for that, the if() runs because the condition is true. If, instead, the checks are reversed and it checks for codes first (like in pull request #81), it does not conflict with zii because there is already a "z" key in codes.

All that to say...swapping those two fixes the conflict.

[lidel]

Well, this is the type of second-degree problems mutating Object prototype (what zii does) gets you into 🤷

IIUC we could accept a PR that protects our libs from this malevolence and adds additional check in:

js-multibase/src/index.js

Lines 116 to 122 in c373031

	function encoding (nameOrCode) {
	if (constants.names[/** @type {BaseName} */(nameOrCode)]) {
	return constants.names[/** @type {BaseName} */(nameOrCode)]
	} else if (constants.codes[/** @type {BaseCode} */(nameOrCode)]) {
	return constants.codes[/** @type {BaseCode} */(nameOrCode)]
	} else {
	throw new Error(`Unsupported encoding: ${nameOrCode}`)

Something like:

- if (constants.names[nameOrCode]) { 
+ if (constants.names[nameOrCode] && typeof constants.codes[nameOrCode] === 'string') {

Thoughts?

[KyleMaas]

Well, this is the type of second-degree problems mutating Object prototype (what zii does) gets you into

You'll hear no argument from me on that. But unfortunately, it's a dependency that's buried rather deep within other stuff. I do have an open issue filed with that project to try to eliminate zii, but they use it a lot, so it's probably going to be quite a while before that happens.

IIUC we could accept a PR that protects our libs from this malevolence and adds additional check in:

js-multibase/src/index.js

Lines 116 to 122 in c373031

	function encoding (nameOrCode) {
	if (constants.names[/** @type {BaseName} */(nameOrCode)]) {
	return constants.names[/** @type {BaseName} */(nameOrCode)]
	} else if (constants.codes[/** @type {BaseCode} */(nameOrCode)]) {
	return constants.codes[/** @type {BaseCode} */(nameOrCode)]
	} else {
	throw new Error(`Unsupported encoding: ${nameOrCode}`)

Something like:

- if (constants.names[nameOrCode]) { 
+ if (constants.names[nameOrCode] && typeof constants.codes[nameOrCode] === 'string') {

Thoughts?

That should work. Or pull request #81, which I tested with zii to make sure it works.

[KyleMaas]

However it gets fixed, if you want to test it, all you have to do is npm install zii and add require("zii") to a source file, then run the test suite.

[KyleMaas]

Something like:

- if (constants.names[nameOrCode]) { 
+ if (constants.names[nameOrCode] && typeof constants.codes[nameOrCode] === 'string') {

Thoughts?

Turns out this didn't quite work. (a) It needs to check constants.names in the second part, and (b) the elements of that are instances of Base, so it needs to check for an object type. At any rate, I've submitted a working version (which I tested both with and without zii) as #82.

[hugomrdias]

damn i dont even know what to say @multiformats/javascript-team thoughts ?

[rvagg]

Well ... the correct response is to say that zii is a bad actor and is going to cause side-effects all over the place and you really need to reject it from your dependency tree as fast as you can as if it's a viral infection you need to treat.

But we could deal with it directly without too much pain though and won't feel like a hack because it's technically appropriate anyway:

diff --git a/src/index.js b/src/index.js
index df66bad..8bda372 100644
--- a/src/index.js
+++ b/src/index.js
@@ -114,9 +114,9 @@ function validEncode (name, buf) {
  * @throws {Error} Will throw if the encoding is not supported
  */
 function encoding (nameOrCode) {
-  if (constants.names[/** @type {BaseName} */(nameOrCode)]) {
+  if (constants.names.hasOwnProperty(/** @type {BaseName} */(nameOrCode))) {
     return constants.names[/** @type {BaseName} */(nameOrCode)]
-  } else if (constants.codes[/** @type {BaseCode} */(nameOrCode)]) {
+  } else if (constants.codes.hasOwnProperty(/** @type {BaseCode} */(nameOrCode))) {
     return constants.codes[/** @type {BaseCode} */(nameOrCode)]
   } else {
     throw new Error(`Unsupported encoding: ${nameOrCode}`)

(edit: the types are probably not going to be happy here, maybe good enough to just remove the type cast although I'm not sure what the flow-on effects will be)

[KyleMaas]

Well ... the correct response is to say that zii is a bad actor and is going to cause side-effects all over the place and you really need to reject it from your dependency tree as fast as you can as if it's a viral infection you need to treat.

But we could deal with it directly without too much pain though and won't feel like a hack because it's technically appropriate anyway:

diff --git a/src/index.js b/src/index.js
index df66bad..8bda372 100644
--- a/src/index.js
+++ b/src/index.js
@@ -114,9 +114,9 @@ function validEncode (name, buf) {
  * @throws {Error} Will throw if the encoding is not supported
  */
 function encoding (nameOrCode) {
-  if (constants.names[/** @type {BaseName} */(nameOrCode)]) {
+  if (constants.names.hasOwnProperty(/** @type {BaseName} */(nameOrCode))) {
     return constants.names[/** @type {BaseName} */(nameOrCode)]
-  } else if (constants.codes[/** @type {BaseCode} */(nameOrCode)]) {
+  } else if (constants.codes.hasOwnProperty(/** @type {BaseCode} */(nameOrCode))) {
     return constants.codes[/** @type {BaseCode} */(nameOrCode)]
   } else {
     throw new Error(`Unsupported encoding: ${nameOrCode}`)

I would agree with this sentiment, and have expressed such in the bug report for the project that pulls it in. But, it's a core package for JavaScript Scuttlebutt clients, so it's not like we can just drop it and it would be difficult for us to reimplement. So...it is what it is.

That change looks to me like it should work. I haven't tested it, but it looks like it would solve the problem.