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

feat: Add streaming support using vinyl-contents #200

Merged
merged 4 commits into from
Jun 9, 2021
Merged

Conversation

phated
Copy link
Member

@phated phated commented May 9, 2019

@demurgos here is a really quick implementation (with only 1 test) of adding streaming support. Going forward, I think this pattern is going to be the recommended pattern to support streaming in plugins that wrap a module that doesn't support streaming.

I didn't want to try to redo everything since I know you are working on the test updates and I'll have to rebase this when completed.

Closes #198

@phated phated requested a review from demurgos May 9, 2019 12:58
@phated
Copy link
Member Author

phated commented May 9, 2019

Note that putting this into a pipeline converts from streaming contents to buffered contents, which might be bad for some people but it's better than just crashing on streaming contents.

@phated
Copy link
Member Author

phated commented May 9, 2019

Ref #198

@demurgos
Copy link
Member

demurgos commented May 9, 2019

Thank you for the PR. I'll have to take a closer look at how it works but it seems good.

@phated
Copy link
Member Author

phated commented May 14, 2019

@demurgos Just wanted to follow up here. I'm currently in the process of abstracting this logic into a utility module but ran into some weird edge cases that I'm not sure how to solve. I'm also starting a new job this week and have been overwhelmed with that. I hope to get back to this soon.

@demurgos
Copy link
Member

Okay, thanks for the info. There is no rush. Good luck with your new job 👍

@phated
Copy link
Member Author

phated commented May 16, 2019

Thanks! I had a chance to create a reproduction and submit it to cloneable-readable at mcollina/cloneable-readable#18

@phated phated changed the title Really hacky implementation of streaming support [WIP] streaming support May 16, 2019
@phated phated force-pushed the support-streaming branch from 5091a85 to 617ed07 Compare May 26, 2019 23:16
@phated phated changed the title [WIP] streaming support Streaming support May 26, 2019
@phated
Copy link
Member Author

phated commented May 26, 2019

@demurgos alright, I was finally able to wrap up the vinyl-contents module and fixed the implementation within this module.

This is the first module where I'm trying this approach so we'll need to keep an eye on it before implementing it for other modules.

@phated phated changed the title Streaming support Streaming support (closes #198) May 26, 2019
@phated phated force-pushed the support-streaming branch from 617ed07 to aba9d2d Compare June 9, 2021 03:33
@phated phated changed the title Streaming support (closes #198) feat: Add streaming support using vinyl-contents Jun 9, 2021
@phated phated merged commit 642d1ea into master Jun 9, 2021
@phated phated deleted the support-streaming branch June 9, 2021 03:50
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.

Support streaming contents
2 participants