Fix/err code

[jacobheun]

Required by ipfs/js-ipfs#1557

This leverages the upcoming update to interface-datastore, to use consistent error codes, in order to better handle not found errors from the various databases available to ipfs users.

Once all the releases mentioned in js-ipfs#1557 are completed, all datastores will provide the same code when .get yields a not found error.

I also added an additional check of the result so that we don't attempt to create a CID in the event that a different error is returned from the database and result is null/undefined.

Note: This is not dependent on any other PR, however, if users get this update and not the datastore-level update (or vice versa), ipfs/js-datastore-level#10, they'll get an error if their mfs root doesnt exist yet.

[achingbrain]

Just seen this, sorry for the delay. You could do:

if (error && (error.notFound || error.code === 'ERR_NOT_FOUND')) {

to decouple this change from the dependency on ipfs/js-datastore-level#10 being merged & released, then remove the error.notFound at a later date once the js-datastore-level change has been released.

Otherwise LGTM.

[jacobheun]

Ah yes, that would be a good approach. No worries about not seeing this, I was waiting on some other releases to happen first before I added you as a reviewer to get it on your radar. I just updated that, so it should be good to release this safely.

[jacobheun]

@achingbrain this should be good to merge and release when you have time. Thanks!

[achingbrain]

Just trying to release this - is the upgrade to interface-datastore@0.5.0 necessary? js-ipfs depends on version 0.4.2 so you end up with two versions in node_modules, consequently instanceof on this line fails.

[achingbrain]

I really wish we'd release v1 of these things.

[vasco-santos]

@jacobheun and @achingbrain

I think that we should use the class-is for type checks, which is already been used across many IPFS repos to prevent this problem.

References:

js-ipfs#938#issuecomment-373926328
js-ipfs#1310

[vasco-santos]

@achingbrain regarding the line failure, I made a PR that fixes it yesterday

ipfs/js-ipfs#1558

But we should change things to that class-is when possible

[jacobheun]

So, we can move the interface-datastore version back to 0.4.2 to get tests passing here, but we're going to hit failures for ipfs/js-ipfs#1558. I reverted to 0.4.2 and tested against that ipfs branch and got the same instanceof failure, as some modules are already updated to 0.5.0.

We should upgrade interface-datastore to add class-is, but we are going to hit the same problem until everything creating and checking keys are using class-is supported versions. We'll likely need to release a patch for the interface-datastore 0.4.x line to make this easier.

I think we can revert to 0.4.2 here, and handle the incongruities in the ipfs/js-ipfs#1558 PR. Does that sound reasonable @achingbrain @vasco-santos ?

On a general note, I agree that it would be nice to start using major versions more prevalently. I also would like to see us transition to not using modules we are a dependency of in our base test suites. I think it's great for running pre-release tests to make sure we're not breaking things, and upgrading major versions if we are, but as a baseline for tests it makes upgrading the dependency a huge pain. We do this a lot with IPFS and anytime we have multiple modules using a single dependency it ends up being a nightmare to release and have ci passing.

[achingbrain]

I think we should just wait for ipfs/js-ipfs/pull/1558 to be merged, then we can release this as a new minor so @alanshaw can control when it goes out with js-ipfs. The interface-datastore@0.5.0 upgrade will have to happen eventually.

[jacobheun]

I've tested this code against js-ipfs#1558 and both of these against the s3 custom datastore. That should be fine, but tests here will fail until this module is pulling in a version of ipfs that has the #1558 updates.

[vasco-santos]

yeah, I agree with waiting for ipfs/js-ipfs/pull/1558 to get merged and then update here

[alanshaw]

Unblocked 🏇💨

[achingbrain]

Hooray!

[achingbrain]

@alanshaw are you doing a release with ipfs/js-ipfs#1558 in it?