Skip to content

Migrate Dapr programmatic pubsub functionality into Dapr.Messaging package #1518

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

Open
wants to merge 8 commits into
base: release-1.16
Choose a base branch
from

Conversation

WhitWaldo
Copy link
Contributor

@WhitWaldo WhitWaldo commented Apr 13, 2025

Description

This adds the programmatic pubsub capabilities into Dapr.Messaging. This doesn't remove anything from Dapr.Client and is only additive. However, I've made changes to the shape of the API to simplify it and use more of the modern .NET functionality, so it's not necessarily a 1:1 transition from Dapr.Client to this implementation, but it's very similar. I should write up some documentation discussing this migration...

This PR also introduces a breaking change to Dapr.Messaging. Because I'm adding programmatic and declarative PubSub functionality, I needed to change the name of DaprPublishSubscribeClient to something less exclusive (especially as it supports only streaming subscription capabilities). As such, this has been renamed to DaprPubSubStreamingClient and its various associated types renamed as well. The v1.16 release should mark the last time we need to change this name.

Issue reference

We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.

Please reference the issue this PR will close: #1509

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

  • Code compiles correctly
  • Created/updated tests
  • Extended the documentation

…to a separate client - note that this is a BREAKING CHANGE in the sense that several types have been renamed.

Added programmatic PubSub functionality from Dapr.Client into package.

Signed-off-by: Whit Waldo <[email protected]>
@WhitWaldo WhitWaldo added this to the v1.16 milestone Apr 13, 2025
@WhitWaldo WhitWaldo self-assigned this Apr 13, 2025
@WhitWaldo WhitWaldo requested review from a team as code owners April 13, 2025 06:15
Comment on lines +52 to +55
options.Converters.Add(new Rfc3389JsonConverter());
var serializedTime = JsonSerializer.Serialize(value.Time, options).Trim('"');

writer.WriteString("time", serializedTime);

Choose a reason for hiding this comment

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

This seems problematic.

  1. Adding the converter in a serializer is suspect, better to be plumbed through from above if that's the case.
  2. Serializing the value then trimming it then writing it out as a string is an expensive set of string ops that could be avoided.
  3. The converter is now going to be used during serialization of value.Data and that may be unexpected downstream.

Probably better to just inline the RFC 3389 serialization here for this specific field.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants