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

[Miniflare 3] Add outboundService option for customising global fetch() target #629

Merged
merged 1 commit into from
Jul 15, 2023

Conversation

mrbbot
Copy link
Contributor

@mrbbot mrbbot commented Jul 14, 2023

workerd exposes a globalOutbound option for customising which service global fetch() calls should be dispatched to. For tests, it's useful to be able to mock responses to certain routes or assert certain requests are made. This new option allows fetch() call to be sent to another configured Worker. See the added test for an example.

Note support for undici's MockAgent API in Miniflare 3 is still planned. This is a lower-level primitive for customising fetch() responses.

/cc @vlovich

@mrbbot mrbbot added the tre Relating to Miniflare 3 label Jul 14, 2023
@mrbbot mrbbot requested a review from a team July 14, 2023 21:53
@changeset-bot
Copy link

changeset-bot bot commented Jul 14, 2023

⚠️ No Changeset found

Latest commit: edaef43

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@vlovich
Copy link
Contributor

vlovich commented Jul 14, 2023

Omg. I added so much custom code to emulate this. If I'd know I would have asked for this sooner. LGTM

@kentonv
Copy link
Member

kentonv commented Jul 14, 2023

Note that this setting affects more than just fetch() -- it will also affect where connect() goes, and would presumably affect similar functions that we add in the future. Hence why the setting in workerd is called globalOutbound and not globalFetch.

What you want to name it is up to you but just wanted to make sure this was understood.

@vlovich
Copy link
Contributor

vlovich commented Jul 14, 2023

@mrbbot is there a way to make it easier to have a globalOutbound option that takes a function & sets up the requisite magic worker to tunnel out and invoke the function?

@mrbbot
Copy link
Contributor Author

mrbbot commented Jul 14, 2023

Note that this setting affects more than just fetch()

Ah good to know, thanks! Will rename to outboundService. 👍


@vlovich something like...

new Miniflare({
  async outboundService(request) {
    return new Response(...)
  }
})

Could definitely make it accept any service designator similar to the serviceBindings option 👍

Copy link
Contributor

@penalosa penalosa left a comment

Choose a reason for hiding this comment

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

Looks good! Total bike shedding, but why the change in name from globalOutbound?

`workerd` exposes a `globalOutbound` option for customising which
service global `fetch()` calls should be dispatched to. For tests,
it's useful to be able to mock responses to certain routes or assert
certain requests are made. This new option allows `fetch()` call to
be sent to another configured Worker. See the added test for an
example.

Note support for `undici`'s `MockAgent` API in Miniflare 3 is still
planned. This is a lower-level primitive for customising `fetch()`
responses.
@mrbbot mrbbot force-pushed the bcoll/tre-custom-global-outbound branch from 30b0b47 to edaef43 Compare July 15, 2023 09:01
@mrbbot mrbbot changed the title [Miniflare 3] Add fetchService option for customising global fetch() target [Miniflare 3] Add outboundService option for customising global fetch() target Jul 15, 2023
@mrbbot
Copy link
Contributor Author

mrbbot commented Jul 15, 2023

@penalosa wanted the name to include service to make it clear what the option value was, and match up with serviceBindings since the accepted service designators are the same.

@mrbbot mrbbot requested a review from penalosa July 15, 2023 09:03
@mrbbot mrbbot merged commit 7d4cab7 into tre Jul 15, 2023
8 checks passed
@mrbbot mrbbot deleted the bcoll/tre-custom-global-outbound branch July 15, 2023 10:27
mrbbot added a commit that referenced this pull request Jul 18, 2023
This builds on top of #629, adding back support for `undici`
`MockAgent`s (https://undici.nodejs.org/#/docs/api/MockAgent) via the
`fetchMock` option. This makes it easy to mock outbound `fetch()`
requests in tests. This API is the same as Miniflare 2.
@limzykenneth
Copy link

limzykenneth commented Jul 20, 2023

Has this feature landed in the latest release yet? The documentation on NPM already contains the outboundService option but it does not seem to work yet.

@mrbbot
Copy link
Contributor Author

mrbbot commented Jul 21, 2023

Hey! 👋 This should've been released as part of [email protected]. Which version are you using?

@limzykenneth
Copy link

limzykenneth commented Jul 21, 2023

I doubled checked and turns out I was missing some bindings and fetch is just failing instead.

Sorry another question though, is it possible to filter which request get passed to the outboundService while the rest will route as usual, for example only intercept requests to a certain hostname but for everything else make the network request as usual?

Edit: NVM, I can just return a direct fetch response from the outboundService function. It's not fully the same as coming from Miniflare itself but I'll chalk up any difference to problems with API client libraries I'm using.

mrbbot added a commit that referenced this pull request Jul 21, 2023
This builds on top of #629, adding back support for `undici`
`MockAgent`s (https://undici.nodejs.org/#/docs/api/MockAgent) via the
`fetchMock` option. This makes it easy to mock outbound `fetch()`
requests in tests. This API is the same as Miniflare 2.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tre Relating to Miniflare 3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants