Remove automatic cross-compatibility for hooks/middleware#1316
Draft
brandur wants to merge 1 commit into
Draft
Conversation
This one's related to #1284. In that pull request, we made it so that any hooks sent to `Config.Hooks` that implemented middleware or any middleware sent to `Config.Middleware` that implement hooks were automatically extracted and use for all types. I've been thinking about this for the last couple of days, and in retrospect, I think this automatic compatibility might be a little unnecessarily aggressive. The case in favor is that when adding additional hook-related functionality to something like `otelriver`, the user can just leave in their existing instance in middleware and not change anything to get the existing functionality. (Without the automatic compatibility they'd have to move it to `Config.Plugins` instead.) The case against is that it may result in some surprising behavior if you only intended to add something as either a hook or middleware. We haven't shipped a release containing #1284 yet, so we can still change this if we want to. I didn't ship a strict revert because #1284 also brings in `Config.Plugins` and some useful internal changes to support everything.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This one's related to #1284. In that pull request, we made it so that
any hooks sent to
Config.Hooksthat implemented middleware or anymiddleware sent to
Config.Middlewarethat implement hooks wereautomatically extracted and use for all types.
I've been thinking about this for the last couple of days, and in
retrospect, I think this automatic compatibility might be a little
unnecessarily aggressive. The case in favor is that when adding
additional hook-related functionality to something like
otelriver, theuser can just leave in their existing instance in middleware and not
change anything to get the existing functionality. (Without the automatic
compatibility they'd have to move it to
Config.Pluginsinstead.) Thecase against is that it may result in some surprising behavior if you
only intended to add something as either a hook or middleware.
We haven't shipped a release containing #1284 yet, so we can still
change this if we want to. I didn't ship a strict revert because #1284
also brings in
Config.Pluginsand some useful internal changes tosupport everything.