-
-
Notifications
You must be signed in to change notification settings - Fork 191
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 support for transform option. #82
Conversation
9111214
to
f03a875
Compare
package.json
Outdated
@@ -31,6 +31,7 @@ | |||
"after": "0.8.1", | |||
"istanbul": "0.3.9", | |||
"mocha": "2.2.5", | |||
"replacestream": "~4.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like replacestream is not installable on the default Node.js 0.8 npm. Can you pick a different dependency for the testing, please? Also, please only specify exact versions of dependencies outside of pillarjs/jshttp.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'm looking at using an older version of replacestream
which I think will fix this (a version that has no dependencies specified by caret version).
Awesome! |
f03a875
to
67fd622
Compare
Switched to an older version of |
Gotcha. I would much rather not use some ancient version, because that makes it hard for us to support new versions of Node.js. Can you find a module that is current? Otherwise, I mean, just write a transform stream in the test, because it's pretty simple. |
Basically, just like any module, we always keep our dependencies up-to-date (they are slightly out of date right now, only because we haven't made a commit since those modules released new versions). If we find one of our dependencies no longer work with Node.js 0.8, for example, we replace it with something else that does. If we were to accept the PR as-is it would mean we would right away have a task to figure out what to replace the module with. Ideally, we don't really want to accept PRs that immediately put work on us, I'm sure you'll understand :) |
Heh, no problem - I totally understand. I was just taking the easy way out by using the tests from the PR this was based on 😄. I have an update that I'll push once it is cleaned up that uses a simple, inline replacement for |
67fd622
to
59f9650
Compare
Updated to use latest readable-stream and a trivial replaceStream implementation. |
👍 |
This is a cool feature! Is there any plan to integrate it soon to master? Thanks! |
@dougwilson - are there any plans to take this change? I'm happy to do the work to resolve the conflicts that have arisen after 10 months of sitting stagnant, but only if there is a point. |
I am going to take some liberty here and close this in favor of the much more deeply discussed #195 so that we can consolidate any conversation going forward. We are not going to try and land this feature soon, as we are focusing on the v5 push for |
This is based on PR #69 by @peterbartels (see issue #71), but with conflicts resolved (and some typos in README.md fixed).
Adds a
transform
option, which allows the file stream to be transformed before piping it tores
.