search

gulpjs / vinyl

Virtual file format.
MIT License
1.28k stars 105 forks source link

Breaking: Use cloneable-readable to clone streams - possibly breaking (closes #85) #99

Closed phated closed 8 years ago

phated commented 8 years ago

Don't Merge

@mcollina was this what you were thinking for the usage of cloneable-readable in the #85 discussion? I'm not sure if I implemented with the same vision, any guidance or corrections would be really helpful.

mcollina commented 8 years ago

Yes, exactly. It should be equivalent to the previous approach, but without the memory issues. Plus, it keeps the number of references tracked down.

Let me know how I can help you further, cloneable-readable is an experiment: let me know if you plan to go ahead with that route, I'll keep that up to date and bump it to v1.0.0.

phated commented 8 years ago

@mcollina would you have any suggestions on tests I could add to have further coverage on this?

mcollina commented 8 years ago

I've add a test in https://github.com/gulpjs/vinyl/pull/102.

phated commented 8 years ago

@mcollina I noticed you are handling the on('data') case, but it doesn't seem to handle on('readable'). Thoughts?

mcollina commented 8 years ago

@phated just fixed, update to v0.3.0.

phated commented 8 years ago

@mcollina sorry to keep bugging you, but does the test I added at https://github.com/gulpjs/vinyl/pull/99/commits/a4c3e14ffb8bdafa06824f059799c671b1aef1e6 look correct for readable events? Thanks!

mcollina commented 8 years ago

@phated yes, it's correct.

mcollina commented 8 years ago

This would be a problem, we need to fix it: https://github.com/mcollina/cloneable-readable/issues/1

phated commented 8 years ago

Thanks for all the help on this @mcollina - I'll be bumping major with this change to get further testing in the wild. Once we feel comfortable with cloneable-readable, I'd like it to get bumped to 1.0