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

Contents cannot be a Stream object #140

Closed
demurgos opened this issue Dec 29, 2017 · 1 comment
Closed

Contents cannot be a Stream object #140

demurgos opened this issue Dec 29, 2017 · 1 comment

Comments

@demurgos
Copy link
Member

The following code throws an error:

const Stream = require("stream").Stream;
const file = new Vinyl({contents: new Stream()});
TypeError: Cannot read property 'objectMode' of undefined
  at new Cloneable (node_modules/cloneable-readable/index.js:13:41)
  at Cloneable (node_modules/cloneable-readable/index.js:10:12)
  at File.set (index.js:183:13)
  at new File (index.js:33:17)
  at Context.<anonymous> (test/file.js:173:18)

(see the stack trace at the bottom of this Travis Job)

The error is caused in cloneable-readable when the stream is cloned because it is expected to be a readable stream (with a _readableState property):

function Cloneable (stream, opts) {
  if (!(this instanceof Cloneable)) {
    return new Cloneable(stream, opts)
  }

  var objectMode = stream._readableState.objectMode
  this._original = stream
  this._clonesCount = 1

  opts = opts || {}
  opts.objectMode = objectMode

  Ctor.call(this, opts)

  forwardDestroy(stream, this)

  this.on('newListener', onData)
}

The documentation is only requiring stream contents to only inherit from Stream:

The contents of the file. If options.contents is a Stream, it is wrapped in a cloneable-readable stream.

Either the documentation should have stronger requirements for streams (readable stream) or the implementation should check that the stream is readable before cloning it. There's also #133 but it would require a semver major breaking change.

This error was present in older versions of vinyl and was found while migrating away from gulp-util in gulp-cssnano.

@phated
Copy link
Member

phated commented Dec 29, 2017

Good catch! It definitely needs to be a ReadableStream. require('stream').Stream is a super old legacy API that points to a version of streams before Readable/Writable/Duplex. The readable-stream package actually does things a little differently by exporting a ReadableStream as the main export.

@phated phated closed this as completed in a1c2e82 Dec 29, 2017
demurgos added a commit to demurgos/vinyl that referenced this issue Dec 29, 2017
Fixes the documentation and add some tests for the File constructor.

Closes gulpjs#140
idontsov added a commit to idontsov/gulp-css-base64 that referenced this issue Sep 14, 2020
If the Vinyl.contents is a stream, it must be ReadableStream. See gulpjs/vinyl#140
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

No branches or pull requests

2 participants