Drop class-is dependecy

[Gozala]

Suggestion to drop class-is dependency an implementing isCID locally came from the discussion in moxystudio/js-class-is#25

Submitting issue here for further discussions.

Keep the api just without class-is, implement isCID with instanceof with fallback to validating the components

@hugomrdias what do you mean by validating components ?

At the moment as far as I understand isCID just boils down to:

obj && obj[Symbol.for('@ipld/js-cid/CID')]

Are you suggesting new CID.isCID should be something along these lines:

class CID {
  static typeSymbol: Symbol.for('@ipld/js-cid/CID')
  static isCID(value) {
    return value instanceof CID || (value && value[CID.typeSymbol])
  }
}

Or do you meant something like:

class CID {
  static isCID(value) {
    return value instanceof CID || CIDUtil.checkCIDComponents(other) == null
  }
}

If later that would be pretty wonderful, and step towards #109. On the other hand it may break some assumptions about the availability of certain instance methods.

I think it would be better to avoid such a dramatic change of isCID and just let it be a glorified instanceof check. Instead I would propose to introduce asCID static method as described in #111 (originally I wrote it up here, but decided it would be better to move into separate thread)

[mikeal]

sigh, static isn’t supported widely enough yet. For instance, it’s not in the default eslint grammar which means it isn’t in the default standard grammar 😥, so we’ll need to use Object.defineProperty instead.

In principal I’m all for this. Taking an entire dependency that creates a new class wrapper for what should be a relatively simple symbol convention has always seemed a bit off to me.

[Gozala]

sigh, static isn’t supported widely enough yet. For instance, it’s not in the default eslint grammar which means it isn’t in the default standard grammar 😥, so we’ll need to use Object.defineProperty instead.

Is that still the case ? In my work I have being defining static class methods and have not seen any complaints from aegir.

[mikeal]

We should not move to instanceof checks.

instanceof checks can’t be used reliably when handing class instances to third party consumers because the pointer check won’t work if two versions of a dependency are resolved in the module tree. This leads to very problematic errors that are hard to reproduce because they only happen during a version discrepancy.

In this specific case, we’re also relying on the symbol to avoid transparent errors in the migration to the new CID class in js-multiformats so this would be extra problematic for this module.

And finally, I’ve said this in another thread, but we should definitely not allow things that are not instances of some sort of CID class to pass the isCID check like { ‘/‘: stringEncodedCID }. This will have very broad and painful ramifications across codecs and other consumers. We very much rely on code along the lines of:

if (CID.isCID(value)) {
  fn(value.toString()) // multibase encoded CID
}

If isCID allows another form it will not cause an exception here even though it’s definitely going to be doing the wrong thing.

[mikeal]

Is that still the case ? In my work I have being defining static class methods and have not seen any complaints from aegir.

That’s because aegir uses some other eslint plugins that support the grammer. Here’s what happens with latest standard.

class Test {
  static test = 'asdf'
}
console.log(Test)

$ node static.js
[Function: Test] { test: 'asdf' }
$ standard static.js 
standard: Use JavaScript Standard Style (https://standardjs.com)
  /root/tmp/static.js:2:15: Parsing error: Unexpected token =

[Gozala]

We should not move to instanceof checks.

instanceof checks can’t be used reliably when handing class instances to third party consumers because the pointer check won’t work if two versions of a dependency are resolved in the module tree. This leads to very problematic errors that are hard to reproduce because they only happen during a version discrepancy.

I think it's well understood, I do mention that here and also further point out how isCID pattern does not really address that in #111

In this specific case, we’re also relying on the symbol to avoid transparent errors in the migration to the new CID class in js-multiformats so this would be extra problematic for this module.

I was not proposing to drop symbol check, I did however assumed (incorrectly it seems) that checkCIDComponents was checking for that symbol.

And finally, I’ve said this in another thread, but we should definitely not allow things that are not instances of some sort of CID class to pass the isCID check like { ‘/‘: stringEncodedCID }. This will have very broad and painful ramifications across codecs and other consumers.

I understand that and is not what I am suggesting.

[Gozala]

That’s because aegir uses some other eslint plugins that support the grammer. Here’s what happens with latest standard.

Right I was referring to static methods which are ok, static properties are not.

P.S.: I have being using static getters to work around lack of static properties support.

[mikeal]

I understand that and is not what I am suggesting.

Ok, sorry for my miss-understanding.

[mikeal]

P.S.: I have being using static getters to work around lack of static properties support.

Then the properties aren’t enumerable tho :( which is why I’ve stuck with Object.defineProperty for now.

I can’t actually figure out what the status is of this in standards. The originally proposal went to Stage 2, was then merged into another proposal which was then merged into another proposal which now doesn’t actually mention static class variables 🤷‍♂️

[hugomrdias]

If later that would be pretty wonderful, and step towards #109. On the other hand it may break some assumptions about the availability of certain instance methods.

yes this is what i was suggesting, which assumptions are we breaking ?

[Gozala]

yes this is what i was suggesting, which assumptions are we breaking ?

const {codec, version, multihash, multibaseName }  = new CID(...)
const v = {codec, version, multihash, multibaseName } 
if (CID.isCID(v)) {
   v.toV0() // throws
}

[Gozala]

Above being said, I think it would be better to not rely on instance methods and instead migrate to static methods so above would translated to:

if (CID.isCID(v)) {
  CID.toV0(v)
}

However that is going to be a big change. I think this can be a more pragmatic compromise #111 Better yet would be to do both and internally use static methods. However I don't feel like proposal to use statics got much support.

[mikeal]

Can someone point me to what toV0 is used for?

V0 conversion is so limited (dagpb only) I’m having a hard time figuring out what this is used for in practice.

[Gozala]

Can someone point me to what toV0 is used for?

V0 conversion is so limited (dagpb only) I’m having a hard time figuring out what this is used for in practice.

To be clear I just picked on toV0 but it could be any other method of CID class e.g. toBaseEncodedString or buffer getter or anything really.

[hugomrdias]

Got it, I guess I would like to find a solution that could be applied to all the other modules that use class-is. Something like this https://github.com/feross/is-buffer/blob/master/index.js

Even something that is best effort, would be acceptable and avoid having that ugly wrapper.

[Gozala]

Got it, I guess I would like to find a solution that could be applied to all the other modules that use class-is. Something like this https://github.com/feross/is-buffer/blob/master/index.js

I think following should do pretty much what current implementation does without wrappers and all:

class CID {
  static isCID(value) {
    return value instanceof CID || (value && value[typeSymbol])
  }
}


const typeSymbol = Symbol.for('@ipld/js-cid/CID')

I can submit a pull request if there is a consensus on this.

[hugomrdias]

yeah, sure and we can go a bit further and test for constructor, the isCID method itself and some critical internal data/component that would fail for breaking changes.

@mikeal how are you relying on the symbol for the new multiformats ?