-
-
Notifications
You must be signed in to change notification settings - Fork 107
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
Added File.toJSON
method
#84
Conversation
Cloning the stats object would remove its prototype, meaning it would lose methods normally available on fs.Stats instances, such as Stats.isDirectory() or Stats.isFile() This resulted in errors being thrown whenever a cloned file was piped through to a gulp.dest() stream when it expected the isDirectory() method.
Correct File.clone() treatment of File.stats
Correct README about pipe's end option.
Spotted this because the file had funny syntax highlighting on GitHub.
Travis: Dump node 0.9 - travis-ci/travis-ci#2251
LICENSE: Remove executable mode
LGTM, @phated ? |
var self = this; | ||
return { | ||
basename: self.basename, | ||
contents: self.contents.toString(), |
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.
this doesn't seem to handle streaming or null contents
Just 1 problem and then I need to run it against gulp-debug |
I'm open to suggestion on how to address the |
@Marak The same logic from clone can be used in toJSON - clone also handles custom attributes, streaming contents, null stats, etc. |
I think toJSON is supposed to result in a serializable object |
Yeah idk you can't really serialize a stream synchronously so I guess it'll have to exclude contents if it isn't null or a buffer |
Serialization of stream would require entire stream processed into buffer. So I think maybe method should have option to accept optional callback? I think in most cases the user would want the actual contents of the file. |
An optional callback wouldn't work because the |
Where is You've lost me. Place holders seem okay, but I'd still need a way to serialize the The goal here is to be able to ship a single |
@Marak Sorry, what I meant is that doing |
So I think an optional callback would still work? In the case of |
I'm also wondering, is anyone actually doing |
I think the best way to handle buffering and serializing would be to use https://www.npmjs.com/package/vinyl-buffer like: var buffer = require('vinyl-buffer');
var through = require('through2');
vfs.src('**/*.txt', { buffer: false }) // totally contrived
.pipe(buffer())
.pipe(through.obj(function(file, enc, cb){
cb(null, JSON.stringify(file));
}); I think it makes a lot of sense to just throw on a stream type (or anything non-serializable) which is similar to how |
Currently, calling |
I see. I'll take a look at I've got my fork working here, so I'll continue to push on the use-case I require. I'll keep a watch on this issue and will post any more code changes I have to make on my fork. Cheers. |
After discussions and reading the spec, throwing a TypeError seems wrong/bad. As per @terinjokes, it's looking like nulling |
Let's make sure to document this, and how a user can convert (and why they shouldn't if necessary) |
If you Is that information important to track? |
More commits will follow. Still working on my integration over here. |
@Marak cool, really interested to see what behavior you need when you get to streaming contents (if you are using vinyl-fs: |
e2a9104
to
896f68d
Compare
Hello. Did a quick PR to resolve issue #83
Let me know what you think. I'm still testing this locally, but it seems OK.
Accidentally did two PRs ( missed a project LINT rule ). Sorry.