Drop class-is dependecy about js-cid HOT 16 OPEN

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.

from js-cid.

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.

from js-cid.

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.

from js-cid.

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 =

from js-cid.

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.

from js-cid.

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.

from js-cid.

I understand that and is not what I am suggesting.

Ok, sorry for my miss-understanding.

from js-cid.

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 🤷‍♂️

from js-cid.

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 ?

from js-cid.

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
}

from js-cid.

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.

from js-cid.

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.

from js-cid.

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.

from js-cid.

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.

from js-cid.

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.

from js-cid.

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 ?

from js-cid.