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

Send options to through2 via dest(), src(), symlink() #107

Merged
merged 18 commits into from
Dec 26, 2015

Conversation

vineethawal
Copy link
Contributor

Ability to send options to through2.
Issue: default value set for highWaterMark in through2 is 16(max buffer length), when buffer length exceeds this default limit all files are not written to destination.

@sindresorhus
Copy link

👎 That would only mask your actual problem. The max buffer is 16, true, but it should flush and continue after that, but something is stopping it. You should rather fix the actual problem.

@phated
Copy link
Member

phated commented Oct 1, 2015

While I agree with @sindresorhus that the pipeline should be fixed (return the stream, call resume, etc). I don't think it is wise for us to not allow a user to pass options to through2. I thought this was already fixed in 2.x though...

@phated
Copy link
Member

phated commented Oct 1, 2015

@vineethawal if we are going to support this, it needs to be uniform across the board, in src, dest, symlink (maybe even in glob-stream?). We aren't only going to support it in dest

@yocontra
Copy link
Member

yocontra commented Oct 1, 2015

@phated Agree, we shouldn't be blocking the users from passing options to the underlying stream. Also, this won't even mask the problem because your plugins will also have a high water mark of 16

@vineethawal
Copy link
Contributor Author

@phated Yes I agree, on it!

@vineethawal vineethawal changed the title Send options to through2 via dest() Send options to through2 via dest(), src(), symlink() Oct 2, 2015
@phated
Copy link
Member

phated commented Oct 2, 2015

@vineethawal looking good. Please update the docs to mention that options are passed to through2 and add some tests. Thanks

@phated phated force-pushed the master branch 2 times, most recently from 4cc2f7d to 73f657e Compare October 16, 2015 01:50
@vineethawal
Copy link
Contributor Author

Added Documentation + test cases

@vineethawal
Copy link
Contributor Author

@phated if the PR looks good can you have this merged?

@vineethawal
Copy link
Contributor Author

@phated can we have this merged? waiting to use this feature in my project

@phated
Copy link
Member

phated commented Nov 19, 2015

@vineethawal I was reviewing this but we have a bigger problem in issue #127 / #128 that needs to get in ASAP. I also noticed that only a couple of the changes are being tested.

@phated
Copy link
Member

phated commented Dec 24, 2015

@vineethawal looks like I messed up the merge with my docs update, could you rebase and get your docs updates to merge cleanly? thanks!

@vineethawal
Copy link
Contributor Author

@phated Done!

@phated
Copy link
Member

phated commented Dec 24, 2015

Awesome. Thanks! I'm busy during the day today but I will take a look tonight

@@ -224,6 +225,10 @@ Type: `Boolean` or `Object`

Default: `undefined` (do not write sourcemaps)

##### other
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should pull the documentation from the through2 repo instead of linking

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I generally agree with surfacing docs but I don't think people should be using these options so I'm good with them not being surfaced.

@phated phated merged commit 80ba8ae into gulpjs:master Dec 26, 2015
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.

4 participants