-
Notifications
You must be signed in to change notification settings - Fork 396
Resolve breaking change to run_as_background_process
in module API
#18737
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
Resolve breaking change to run_as_background_process
in module API
#18737
Conversation
@MadLittleMods out of interest did you see @reivilibre's suggestion here? |
@@ -0,0 +1 @@ | |||
Deprecate `run_as_background_process` exported as part of the module API interface in favor of `ModuleApi.run_as_background_process`. See the relevant section in the upgrade notes for more information. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MadLittleMods out of interest did you see @reivilibre's suggestion here?
No. I just solved it how I've been solving it for all of the metrics refactors, passing it in manually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, given the consistency this makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docs look excellent. Just a minor suggestion below, and a git conflict to resolve.
Thanks for taking this on, this looks good to me.
And a definite warning to our future selves to not expose internals directly. Another common one is feeding the result of datastore
functions directly back in API responses.
…kground_process Conflicts: docs/upgrade.md
Thanks for the review @anoadragon453 🐄 |
Resolve breaking change to
run_as_background_process
in module APIFix #18735
In #18670, we updated
run_as_background_process
to add aserver_name
argument. Because this function is directly exported from the Synapse module API, this is a breaking change to any downstream Synapse modules that userun_as_background_process
.This PR shims and deprecates the existing
run_as_background_process(...)
for modules by providing a stubserver_name
value and introduces a newModuleApi.run_as_background_process(...)
that covers theserver_name
logic automagically.Dev notes
It looks like we already do this for
@cached
Pull Request Checklist
EventStore
toEventWorkerStore
.".code blocks
.