Skip to content

fix: Wrap hooks now correctly receive options passed by outer wrappers#125

Open
mbg wants to merge 5 commits intogr2m:mainfrom
mbg:mbg/fix-multiple-wrappers
Open

fix: Wrap hooks now correctly receive options passed by outer wrappers#125
mbg wants to merge 5 commits intogr2m:mainfrom
mbg:mbg/fix-multiple-wrappers

Conversation

@mbg
Copy link

@mbg mbg commented Feb 18, 2026

I discovered this while working on octokit/plugin-retry.js#661, where there appeared to be no difference between (simplified):

octokit.hook.wrap("request", (req, opts) => req(req, opts));

and

octokit.hook.wrap("request", (req, opts) => req(opts));

I eventually realised that the argument to (in this case) req didn't matter at all and traced the issue down to this library, specifically https://github.com/gr2m/before-after-hook/blob/main/lib/register.js#L23-L25

Since the docs, tests, and examples, all show the options being passed to the wrapped function, I assume that this is not intentional. This PR fixes the problem so that only the outer-most wrapper gets the initial options object and the others get the arguments they are called with. I also added a test for this.

There is potentially the same issue in https://github.com/gr2m/before-after-hook/blob/main/lib/register.js#L13-L15 but I wasn't sure if maybe it was the intended behaviour there. If not, then the following would fix it:

    return name.reverse().reduce((callback, name) => {
      return register.bind(null, state, name, callback);
    }, method)(options);

@mbg mbg mentioned this pull request Feb 18, 2026
4 tasks
@gr2m
Copy link
Owner

gr2m commented Feb 18, 2026

There is potentially the same issue in main/lib/register.js#L13-L15 but I wasn't sure if maybe it was the intended behaviour there. If not, then the following would fix it:

yeah I think this needs to be fixed as well, great catch 👍🏼 Can you update your PR?

@gr2m gr2m changed the title Fix multiple wrappers all getting the initial options fix: Wrap hooks now correctly receive options passed by outer wrappers Feb 18, 2026
@gr2m gr2m enabled auto-merge (squash) February 18, 2026 19:33
auto-merge was automatically disabled February 18, 2026 20:15

Head branch was pushed to by a user without write access

@mbg
Copy link
Author

mbg commented Feb 18, 2026

@gr2m Done, and also added another test for that.

@gr2m gr2m enabled auto-merge (squash) February 19, 2026 18:03
auto-merge was automatically disabled February 19, 2026 18:19

Head branch was pushed to by a user without write access

@gr2m gr2m enabled auto-merge (squash) February 19, 2026 20:02
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

Comments