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 .stream() method #187

Merged
merged 17 commits into from
Mar 5, 2019
Merged

Add .stream() method #187

merged 17 commits into from
Mar 5, 2019

Conversation

bencmbrook
Copy link
Contributor

@bencmbrook bencmbrook commented Feb 22, 2019

This PR adds a method to support Node streams without causing side effects on the state of the stream. It reads in the minimum amount of bytes to detect the filetype, detects the file type, then rolls back the stream for use elsewhere.

Fixes #186

index.js Show resolved Hide resolved
test.js Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

Travis is failing

@sindresorhus sindresorhus changed the title Adds .stream method and closes #186 Add .stream() method Feb 23, 2019
@bencmbrook
Copy link
Contributor Author

Travis is failing

Interesting. Will look into it.

@bencmbrook bencmbrook changed the title Add .stream() method [WIP] Add .stream() method Feb 24, 2019
@bencmbrook
Copy link
Contributor Author

Piping the readableStream through a PassThrough stream is a cleaner way of doing this because it preserves the event lifecycle and the stream flowing mode. Before, it was switching modes, which was causing it to break it Node 10.

This does require that we import the stream node module in index.js. I didn't use the ES6 module syntax there.

The open question then becomes, how do we want to keep this browser compatible? The stream method nor the require should be used in browsers.

@sindresorhus
Copy link
Owner

Piping the readableStream through a PassThrough stream is a cleaner way of doing this because it preserves the event lifecycle and the stream flowing mode. Before, it was switching modes, which was causing it to break it Node 10.

Agreed

The open question then becomes, how do we want to keep this browser compatible? The stream method nor the require should be used in browsers.

Webpack is super annoying in this sense and tries to import every require, even if it's behind an if-statement or not even used. One way to work around this is to use eval:

module.exports.stream = () => {
	const stream = eval('require')('stream');
	// …
};

@bencmbrook bencmbrook changed the title [WIP] Add .stream() method Add .stream() method Feb 24, 2019
index.js Outdated Show resolved Hide resolved
@bencmbrook bencmbrook changed the title Add .stream() method [WIP] Add .stream() method Feb 24, 2019
test.js Outdated Show resolved Hide resolved
test.js Outdated Show resolved Hide resolved
@bencmbrook
Copy link
Contributor Author

Bug found on the .rtf file. .read returns null. Should test all files.

https://github.com/bencmbrook/file-type/blob/929de6c441bc9b0c2cb6642f15a3d239ee7d6470/index.js#L928

@bencmbrook bencmbrook changed the title [WIP] Add .stream() method Add .stream() method Feb 24, 2019
@bencmbrook
Copy link
Contributor Author

Features are done and tests are all passing. Anything you want changed in the latest commits?

@bencmbrook
Copy link
Contributor Author

Marked everything as resolved

@sindresorhus sindresorhus merged commit 4fb22b3 into sindresorhus:master Mar 5, 2019
@bencmbrook
Copy link
Contributor Author

🤙sweet. I'm gonna get a lot of use out of this.

Also, minor docs error on that readme commit: e8d4e1f#r32592498

@intellix
Copy link

PassThrough causes issues with rewind for me: #216

@bencmbrook
Copy link
Contributor Author

How so? What version of file-type are you using?

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.

Add method that returns Node stream back to normal state
3 participants