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

chore: expose transaction context propagation to the server sdk only #590

Merged
merged 3 commits into from
Oct 24, 2023

Conversation

beeme1mr
Copy link
Member

This PR

  • removes transaction context propagation from web SDK
  • refactored the transaction context propagation logic to live under the server SDK

Notes

Transaction context propagation only makes sense on the server and was simply ignored by the client. This removes the methods from the client sdk to avoid confusion.

This is a breaking change in the web SDK. However, it's sub 1.0 and this feature effectively had no effect if it was used. Currently, this is not marked as a breaking change to avoid releasing a major version. @toddbaert do you know if it's possible to mark the server sdk changes as a chore and the web sdk changes as a breaking fix?

Signed-off-by: Michael Beemer [email protected]

@beeme1mr beeme1mr requested a review from a team as a code owner October 11, 2023 17:20
@toddbaert
Copy link
Member

@toddbaert do you know if it's possible to mark the server sdk changes as a chore and the web sdk changes as a breaking fix?

I don't think there's a way to do this other than manually editing the release PR, so that's the way I'd handle this. It wouldn't be hard - just manual changes to the PR description, the release notes, and the version number.

@beeme1mr
Copy link
Member Author

Hey @toddbaert @lukas-reining @thomaspoignant could one of you take a look at this PR when you have a moment?

Copy link
Member

@lukas-reining lukas-reining left a comment

Choose a reason for hiding this comment

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

The code change looks good to me and makes sense!

@beeme1mr beeme1mr added this pull request to the merge queue Oct 24, 2023
Merged via the queue into main with commit 2cdf175 Oct 24, 2023
7 checks passed
@beeme1mr beeme1mr deleted the move-request-context branch October 24, 2023 14:50
Comment on lines +101 to +103
,
"../server/src/transaction-context"
],
Copy link
Member

Choose a reason for hiding this comment

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

Sorry I'm late to this PR, but I'm a bit confused by this.

It seems odd (and maybe even problematic?) that we need to compile source from server in shared.

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.

3 participants