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

Add connection and context to connected hook payload #607

Merged
merged 1 commit into from
May 20, 2023

Conversation

canadaduane
Copy link
Contributor

@canadaduane canadaduane commented May 16, 2023

Adds connection and context to the connectedPayload type for the connected hook.

Because connectedPayload already has connection (ConnectionConfiguration), if this PR intention is acceptable, we have the choice of either:

(a) make the most sensible change, but require a breaking change (i.e. bump the major version)
(b) make the least disruptive change, but add a key that could be a little confusing / misdirecting

This PR currently applies choice (a), but it could easily be switched. See #606.

@janthurau
Copy link
Collaborator

janthurau commented May 18, 2023

Thanks for this!

Let's take the 2nd choice for now (no BC break), maybe we can just name it connectionInstance (or you have a better idea :)). The naming won't be that nice but I don't want to bump the major just because we rename one variable.. Maybe you can add an @todo so we rename it in the next major (that might be with subdocs, #583). Happy to merge then!

@canadaduane
Copy link
Contributor Author

maybe we can just name it connectionInstance

add an @todo so we rename it in the next major

Done! Thanks for the feedback.

@janthurau janthurau merged commit fb9a24a into ueberdosis:main May 20, 2023
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.

2 participants