Refresh

[kemitchell]

@mafintosh, this PR:

• bumps dependencies
• adopts Standard style
• addresses Standard lint errors
• configures Travis CI to test on latest Node.js

There is an abstract-blob-store test failure: When a file isn't found, .notFound isn't set on the ENOENT error. I couldn't think of a good way to wrap emission of that error offhand. Perhaps you can.

[bcomnes]

LGTM except for that one comment. Thanks!

[bcomnes]

Any idea what the failing test is?

[kemitchell]

See above:

There is an abstract-blob-store test failure: When a file isn't found, .notFound isn't set on the ENOENT error. I couldn't think of a good way to wrap emission of that error offhand. Perhaps you can.

[bcomnes]

Looks perfect except for that failing test. I don't have enough recent context to have a good answer off hand.

[bcomnes]

What would be the correct fix in your opinion?

[kemitchell]

@bcomnes, to be clear, the test breakage is not a result of this PR. master will fail that test if you rerun CI.

When fs.createReadStream() returns an error with error.code === 'ENOENT', the library should set error.notFound = true. I am not sure how to go about wrapping the stream to achieve that.

[bcomnes]

I'm not sure off hand either. Would need to dig in. Put it on my todo, but it might be a while before I have time to look. We can land as soon as we're green.

[kemitchell]

@bcomnes can we land and publish now, since master is already red?

[bcomnes]

We can get this to land but let's try to fix first. If you need immediate results use the kemitchell/content-addressable-blob-store reference in your package.json.

[bcomnes]

I just ran a bisect and found the first bad commit in:

6ecdb25

When it went from abstract blob store 2 - 3.

[bcomnes]

Pinning semver, it appears the tests begin to fail as soon as abstract blob store 3.3.2 is released, so it appears a breaking change was introduced there.

[bcomnes]

Pinning "abstract-blob-store": "~3.2.0" fixes tests.

[bcomnes]

Upstream introduced breaking changes in a semver minor patch: max-mapper/abstract-blob-store#39

Let's get the fix in there and then land when green.

[bcomnes]

LGTM

[bcomnes]

[email protected]

[kemitchell]

@bcomnes, pinning the test suite back at an old version seems wrong to me. If content-addressable-blob-store is meant to be an abstract-blob-store, it ought to pass the latest test suite.

[bcomnes]

It’s not pinned. I fixed upstream. If abs wants to add that error feature it needs to go in as a major. I’m not convinced it’s a good idea though.

[kemitchell]

Ah, I see. You pinned here for testing, but you published a new patch release of abstract-blob-store.

I was relying on .notFound from abstract blob stores.

[bcomnes]

It should probably go back in then, but as a major release.

[bcomnes]

Concrete abstract stores have always been free to provide types errors or error identifiers as well. Having a standard not found might be worth it though, especially since it was added for about a year before anyone noted the breaking nature of it.

[kemitchell]

I strongly agree that `.notFound` is a good idea. The store isn’t really abstract if you have to check for multiple implementation-specific error properties like `error.code === ‘ENOENT’`.

[bcomnes]

Ok well do a 4.0 abs then patch here

[bcomnes]

Opened up a pr to addressed that max-mapper/abstract-blob-store#41