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

[Core AMQP] V2 - Remove DefaultDataTransformer from the exports #12320

Closed

Conversation

HarshaNalluru
Copy link
Member

@HarshaNalluru HarshaNalluru commented Nov 5, 2020

Part of V2 Breakign changes #12116

@@ -2,6 +2,11 @@

## 2.0.0 (Unreleased)

### Breaking changes

- `DefaultDataTransformer` is no longer exported.
Copy link
Contributor

Choose a reason for hiding this comment

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

Not exporting the DefaultDataTransformer, but continuing to export DataTransformer interface does not give us much. The main reason for us to do this is to keep the option open in the future to implement the "serializer" in any other way.

I would recommend removing the dataTransformer property from ConnectionContextBase to allow us to stop exporting this interface from core-amqp. This will result in copying the code in the DefaultDataTransformer to Service Bus and EventHubs and use it like a static class instead of using it from the context.

In the future when we have the common code between SB & EH without needing to expose it users from core-amqp, we can move this

ghost pushed a commit that referenced this pull request Nov 10, 2020
…mqp to client packages (#12415)

Part of the list of breaking changes to core-amqp v2 in #12116
Replaces #12320 (precipitated by #12320 (comment))

This change moves the `DataTransformer` interface and `DefaultDataTransformer` class to service-bus and event-hub packages.

When we establish what our data serde strategy is, we can revisit using a shared common serde solution.
@HarshaNalluru
Copy link
Member Author

Closing in favour of Chris's PR for the same!

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