-
-
Notifications
You must be signed in to change notification settings - Fork 105
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
Breaking: Use cloneable-readable to clone streams - possibly breaking (closes #85) #99
Conversation
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, |
@mcollina would you have any suggestions on tests I could add to have further coverage on this? |
|
||
if (isStream(val)) { | ||
val = cloneable(val); | ||
} |
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.
Are you sure you need this? I would avoid, given you are already wrapping on demand https://github.com/gulpjs/vinyl/pull/99/files#diff-168726dbe96b3ce427e7fedce31bb0bcR85.
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.
Forget it, it's needed, I haven't looked at this for a bit of time.
I've add a test in #102. |
@mcollina I noticed you are handling the |
@phated just fixed, update to v0.3.0. |
a67b085
to
8a38ae1
Compare
cde9727
to
7d8af8c
Compare
@phated yes, it's correct. |
This would be a problem, we need to fix it: mcollina/cloneable-readable#1 |
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 |
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.