Skip to content
This repository has been archived by the owner on Mar 23, 2023. It is now read-only.

fix: prevent delete from sending multiple args in callback #16

Merged
merged 2 commits into from
May 28, 2018

Conversation

jacobheun
Copy link
Contributor

This fixes an issue in node10 where unlink could call the callback as callback(err, undefined). This resulted in test suite failures for ipfs-repo as it was attempting to use async/waterfall.

This PR simply enforces the callback only ever returning an error, if one exists.

The following chain would cause the callback being sent to datastore.has to undefined.

        waterfall([
          (cb) => repo.datastore.delete(b, cb),
          (cb) => repo.datastore.has(b, cb)
        ], (err, exists) => {
          expect(err).to.not.exist()
          expect(exists).to.equal(false)
          done()
        })

This is related to the following issues, but is not the core cause, it is just causing test suite failures in node v10.
relates to ipfs/js-ipfs#1347
relates to ipfs/js-ipfs-repo#167

License: MIT
Signed-off-by: Jacob Heun <jacobheun@gmail.com>
License: MIT
Signed-off-by: Jacob Heun <jacobheun@gmail.com>
@jacobheun
Copy link
Contributor Author

CI has been resolved, this should be good to get merged.

@jacobheun
Copy link
Contributor Author

FYI, the underlying node issue has been filed here: nodejs/node#20872

@jacobheun
Copy link
Contributor Author

@dignifiedquire @diasdavid can we get this released? This update is being used and is needed for the js-ipfs node 10 fixes.

@daviddias daviddias merged commit e98697e into master May 28, 2018
@ghost ghost removed the status/in-progress In progress label May 28, 2018
@daviddias
Copy link
Member

you got it! Released with 0.5.0

@daviddias daviddias deleted the fix/node10 branch May 28, 2018 05:21
@jacobheun jacobheun mentioned this pull request May 29, 2018
10 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants