Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add custom stream support #1825

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

KristapsR
Copy link

@KristapsR KristapsR commented Jun 21, 2024

Description

There are situations when we need to send response stream that is not an instance of node Readable stream and current way of checking for stream value instanceof Stream fails.
e.g. when using alternative module like readable-stream or some other cases

My usecase

import archiver from 'archiver'

const archive = archiver('zip')
const stream = ...
archive.append(stream, { name: 'archive.zip'})

res.body = archive

Solution

  • Added tests to reproduce issue with mocked custom stream object that represents possible Readable stream implementation.
  • Addopted code from is-stream

Tested also with this code

const archiver = require('archiver')
const Stream = require('stream')
const CustomStream = require('./test-helpers/stream.js')
const isReadableStream = require('./lib/is-readable-stream.js')

const stream = new Stream()
const archive = archiver('zip')
const customstream = new CustomStream.Readable()

console.log(stream instanceof Stream) // true
console.log(customstream instanceof Stream) // false
console.log(archive instanceof Stream) // false

console.log(isReadableStream(stream)) // true
console.log(isReadableStream(customstream)) // true
console.log(isReadableStream(archive)) // true

@KristapsR KristapsR changed the title feat: support custom streams Add custom stream support Jun 21, 2024
@KristapsR KristapsR marked this pull request as ready for review June 21, 2024 14:34
@KristapsR KristapsR force-pushed the custom-stream-response branch from 683f127 to 1fc1825 Compare August 13, 2024 20:19
@jonathanong
Copy link
Member

why not use is-stream directly? because it is in es6?

@KristapsR
Copy link
Author

why not use is-stream directly? because it is in es6?

I wasn't sure it is acceptable to introduce additional dependencies.

@jonathanong
Copy link
Member

an external dependency is preferred if maintained from a reliable author, I can try to look later

@kevinpeno
Copy link
Contributor

@jonathanong I reviewed the library in question. Considering we're planning to move into the TS world, while I don't tend to prefer libraries that use d.ts instead of being natively written in TS, the way they have added a d.ts test file ensures that it isn't lying about its types. I'd move for inclusion if that's the preference.

@jonathanong
Copy link
Member

that's a good point, thanks! cc @kevinpeno

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants