chore(streams): replace transform streams with minipass

[claudiahdz]

This change replaces stream.Transform with minipass and refactors the integrityStream fn to call the new IntegrityStream class.

[isaacs]

This is a good start at a very targeted change that disrupts as little as possible of its surrounding code.

I think we can take the improvement a bit further, but it shouldn't block landing this, assuming tests pass.

I believe that this is a breaking change, since it may subtly change the timing of data processing, but should be low-risk to start incorporating into the CLI's dep tree.

As far as performance benchmarking, that'd be a good addition, but I don't think that anything is set up already. We may see only a tiny improvement in this library, but I suspect we'll see more of an impact in pacote and cacache, when many integrity streams are in use.

[isaacs]

This can be a bit tighter.

Since we're only using the class here, you can pass in hashes in the options object, and do this.hashes = opts.hashes. Then, in the write method, this.hashes.forEach(h=>h.update(data)); return super.write(data).

Then the call below becomes const stream = new Transform({ hashes }).

Maybe the class name could be IntegrityStream, also.

In fact, while we're at it, the stream.on('end') bit below could be pulled in as well, and made more resilient against stream.removeAllListeners('end'), by overriding the emit method. Something like this:

  emit (ev, data) {
    if (ev !== 'end') {
      return super.emit(ev, data)
    }
    // all the integrity checking stuff
    super.emit('end')
  }

[isaacs]

Oh, I guess you'd have to also move most of the start of the function into the IntegrityStream as well, so that it has all the other data that the checking needs.

That's probably also a good idea :)

[isaacs]

If the rest of the stuff in this method moves into the stream subclass, maybe this whole function is just:

function integrityStream (opts) {
  return new IntegrityStream(SsriOpts(opts))
}

[isaacs]

This raises an eyebrow for me. Won't this mean that end is emitted twice (or more?)

It'd be safer to override the emit function, I think, to ensure that it can't be removed.

emit (ev, data) {
  if (ev === 'end')
    this.onEnd()

  return super.emit(ev, data)
}

[isaacs]

Can just be this.emit('error', err)

[isaacs]

💯