Skip to content

[Feat]: Add support for streams pipelines #418

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

Merged
merged 45 commits into from
May 14, 2025

Conversation

sleipnir
Copy link
Collaborator

@sleipnir sleipnir commented May 9, 2025

This closes #270

@sleipnir sleipnir marked this pull request as ready for review May 9, 2025 19:57
@sleipnir
Copy link
Collaborator Author

sleipnir commented May 9, 2025

@polvalente, I think this is ready for review now. I would like to rename the modules:

GRPC.Server.Stream -> GRPC.Server.Materializer
and maybe their client-side equivalent.

Justification: I don't think they are a real Stream structure and in the new proposal they are only used to "materialize" the response to the client. It also looks a bit strange to have two modules named Stream in the library.
But I haven't gone ahead with this yet, waiting for your opinion.

The PR was a bit big but sorry there wasn't much that could be done since it's a likely breaking change.

Copy link
Contributor

@polvalente polvalente left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry about the lengthy review. This overall is looking good, but seeing that we're aiming for this to mark v1.0, I think we can commit to the breaking change of merging the GRPC.Stream and GRPC.Server.Stream structs, and remove other kinds of streaming, even if we have to do this through multiple PRs.

Adriano Santos and others added 17 commits May 9, 2025 23:24
Co-authored-by: Paulo Valente <16843419+polvalente@users.noreply.github.com>
Co-authored-by: Paulo Valente <16843419+polvalente@users.noreply.github.com>
Co-authored-by: Paulo Valente <16843419+polvalente@users.noreply.github.com>
Co-authored-by: Paulo Valente <16843419+polvalente@users.noreply.github.com>
Co-authored-by: Paulo Valente <16843419+polvalente@users.noreply.github.com>
Co-authored-by: Paulo Valente <16843419+polvalente@users.noreply.github.com>
Co-authored-by: Paulo Valente <16843419+polvalente@users.noreply.github.com>
@sleipnir
Copy link
Collaborator Author

@polvalente shall we continue?

Copy link
Contributor

@polvalente polvalente left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some minor comments and questions, but this looks mostly good to go (given the pending stuff we're leaving for follow-up PRs)

sleipnir and others added 9 commits May 14, 2025 16:36
Co-authored-by: Paulo Valente <16843419+polvalente@users.noreply.github.com>
Co-authored-by: Paulo Valente <16843419+polvalente@users.noreply.github.com>
Co-authored-by: Paulo Valente <16843419+polvalente@users.noreply.github.com>
Co-authored-by: Paulo Valente <16843419+polvalente@users.noreply.github.com>
Co-authored-by: Paulo Valente <16843419+polvalente@users.noreply.github.com>
Co-authored-by: Paulo Valente <16843419+polvalente@users.noreply.github.com>
Co-authored-by: Paulo Valente <16843419+polvalente@users.noreply.github.com>
Co-authored-by: Paulo Valente <16843419+polvalente@users.noreply.github.com>
Co-authored-by: Paulo Valente <16843419+polvalente@users.noreply.github.com>
@sleipnir
Copy link
Collaborator Author

Just some minor comments and questions, but this looks mostly good to go (given the pending stuff we're leaving for follow-up PRs)

Great! I accepted all suggestions except two so we could discuss them further.

@sleipnir sleipnir merged commit 78b8386 into elixir-grpc:master May 14, 2025
7 checks passed
@sleipnir sleipnir deleted the feat/new-streams-api branch May 14, 2025 20:39
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.

New API Proposal
2 participants