fix(tarball): Remove promise handler error

[redonkulus]

Fixes #145

[zkat]

I think you wanna add the return null to https://github.com/redonkulus/pacote/blob/4d671a853aff5632db11bae9b68ac62544a25e11/lib/fetchers/registry/tarball.js#L75 as well? It's a weird-ass warning anyway and it seems to get tripped up kinda randomly.

[redonkulus]

@zkat Ok I've added one there too. I rebased the commits into 1 to make the history cleaner.

[zkat]

I'm going to assume this fixes it, because I literally can't repro this problem by hand. @redonkulus were you getting the issue yourself? Does this fix it for you? Do you have a reliable repro?

[redonkulus]

@zkat So I'm using a repo that I have lying around. The warning goes away after adding the return null line.

However, I do see the following error now (removing the return null removes the error). The installation does finish successfully as far as I know.

events.js:182░░░░░░⸩ ⠋ extract:supports-color: sill extract eslint-visitor-keys@1.0.0
      throw er; // Unhandled 'error' event
      ^

Error: write after end
    at writeAfterEnd (_stream_writable.js:236:12)
    at PassThrough.Writable.write (_stream_writable.js:287:5)
    at PassThrough.Writable.end (_stream_writable.js:552:10)
    at ReadEntry.entry.on (/Users/redonkulus/.nvm/versions/node/v8.6.0/lib/node_modules/npm/node_modules/pacote/lib/extract-stream.js:19:41)
    at emitOne (events.js:120:20)
    at ReadEntry.emit (events.js:210:7)
    at ReadEntry.emit (/Users/redonkulus/.nvm/versions/node/v8.6.0/lib/node_modules/npm/node_modules/tar/node_modules/minipass/index.js:287:25)
    at ReadEntry.[maybeEmitEnd] (/Users/redonkulus/.nvm/versions/node/v8.6.0/lib/node_modules/npm/node_modules/tar/node_modules/minipass/index.js:240:12)
    at ReadEntry.end (/Users/redonkulus/.nvm/versions/node/v8.6.0/lib/node_modules/npm/node_modules/tar/node_modules/minipass/index.js:153:27)
    at Unpack.[consumeBody] (/Users/redonkulus/.nvm/versions/node/v8.6.0/lib/node_modules/npm/node_modules/tar/lib/parse.js:210:13)
    at Unpack.[consumeChunkSub] (/Users/redonkulus/.nvm/versions/node/v8.6.0/lib/node_modules/npm/node_modules/tar/lib/parse.js:391:40)
    at Unpack.[consumeChunk] (/Users/redonkulus/.nvm/versions/node/v8.6.0/lib/node_modules/npm/node_modules/tar/lib/parse.js:360:30)
    at Unzip.(anonymous function).on.chunk (/Users/redonkulus/.nvm/versions/node/v8.6.0/lib/node_modules/npm/node_modules/tar/lib/parse.js:291:59)
    at emitOne (events.js:115:13)
    at Unzip.emit (events.js:210:7)
    at Unzip.emit (/Users/redonkulus/.nvm/versions/node/v8.6.0/lib/node_modules/npm/node_modules/tar/node_modules/minipass/index.js:287:25)

npm notice created a lockfile as package-lock.json. You should commit this file.
added 608 packages from 704 contributors in 49.235s

To reproduce it, I had to clear the npm cache and remove node_modules each time:

$ npm cache clean --force && rm -rf node_modules package-lock.json && npm i

$ node -v
v8.6.0
$ npm -v
5.8.0

[zkat]

Oh god why

[redonkulus]

Maybe @benjamingr has an idea...

[benjamingr]

You can return Promise.resolve() which is the same as returning undefined, but I don't think the error is related to the return null at all to be honest :D

[redonkulus]

I tried returning Promise.resolve() but that caused even more errors. Using return null, sometimes the error happens sometimes it doesn't. Even when the error does occur, the packages are installed... not sure if that helps.

[benjamingr]

@redonkulus from bluebird's point of view other than warnings - there is no difference between not returning, returning undefined and returning Promise.resolve(). Your princess is in another castle.

[zkat]

@redonkulus could you rebase this branch onto latest, which has patches related to end-of-stream usage, and see if you can still repro the crash?

[redonkulus]

@zkat rebased and tested this locally, looks like the write after end issue is not happening now. It would be good to get someone else to confirm this as well before merging/releasing.

[zkat]

@redonkulus sounds good! I'm gonna have to purge mississippi from everything -- the whole point was to be able to trust its streams, but it's pulled this kinda garbage enough that I want nothing to do with it anymore.

[zkat]

I'm just gonna merge this. It doesn't break anything anyway, and it'll at least fix you.