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

reply callback function argument's body does not contain request body string, but AsyncGenerator #1756

Closed
robertcepa opened this issue Nov 5, 2022 · 4 comments
Labels
bug Something isn't working

Comments

@robertcepa
Copy link

robertcepa commented Nov 5, 2022

Bug Description

According to https://undici.nodejs.org/#/docs/best-practices/mocking-request?id=reply-with-data-based-on-request, the argument provided to the callback function for reply should contain body property containing the request body string. However, body contains an AsyncGenerator, which cannot be used to act upon, because the reply's callback function is not awaited (code does not expect async callback).

Reproducible By

fetchMock.get('...')
  .intercept({
    method: () => true,
    path: () => true,
  })
  .reply(200, (opts) => {
    console.log(opts);
    return opts;
  });

Expected Behavior

opts.body provides the request body string.

Logs & Screenshots

console.log(opts) displays:

{                                                                                                                                                                                                                                                                                                                                                                               
      path: '/some-path',
      origin: 'https://someorigin.dev',
      method: 'POST',
      body: Object [AsyncGenerator] {},
      headers: {
        'content-type': 'application/json',
        'user-agent': '__name__/__version__',
        'mf-loop': '1',
        'content-length': '13',
        'accept-encoding': 'br, gzip, deflate'
      },
      maxRedirections: 0,
      bodyTimeout: 300000,
      headersTimeout: 300000
}

I expect it to display:

{                                                                                                                                                                                                                                                                                                                                                                               
      path: '/some-path',
      origin: 'https://someorigin.dev',
      method: 'POST',
      body: '{"foo":"bar"}',
      headers: {
        'content-type': 'application/json',
        'user-agent': '__name__/__version__',
        'mf-loop': '1',
        'content-length': '13',
        'accept-encoding': 'br, gzip, deflate'
      },
      maxRedirections: 0,
      bodyTimeout: 300000,
      headersTimeout: 300000
}

I'm able to access request body data with the following code, but can't actually use it because the reply's callback is not awaited.

.reply(200, (opts) => {
    (async () => {
          const buffers = [];
          for await (const data of opts.body) {
            buffers.push(data);
          }
          console.log(Buffer.concat(buffers).toString('utf8'));
    })();
    
    return opts;
  });

Environment

OS: Windows 11 Home 22000.1098, macOS Monterey 12.4
NodeJS: v16.18.0

Additional context

undici is a dependency of https://github.com/cloudflare/miniflare that I use to test Cloudflare Workers. miniflare provides MockAgent from undici directly (see: https://github.com/cloudflare/miniflare/blob/31792ba7d98f0aba839d28e13228671c46535b97/packages/core/src/standards/http.ts#L819)

@robertcepa robertcepa added the bug Something isn't working label Nov 5, 2022
@mcollina
Copy link
Member

mcollina commented Nov 5, 2022

Can you provide steps to reproduce? We often need a reproducible example, e.g. some code that allows someone else to recreate your problem by just copying and pasting it. If it involves more than a couple of different file, create a new repository on GitHub and add a link to that.

By the looks of it there are two bugs:

  1. we should allow async functions in reply()

  2. update the docs to tell the body might not contain the full body.
    Thanks for reporting!

@robertcepa
Copy link
Author

robertcepa commented Nov 5, 2022

Full code here: https://github.com/robertcepa/toucan-js/blob/undici-issue-1756/test/index.spec.ts#L23

Steps:

  1. git clone git@github.com:robertcepa/toucan-js.git
  2. git checkout undici-issue-1756
  3. yarn && yarn test (or npm install && npm run test)

We are looking for the output of console.log that shows before the test report from jest.

Context: the file is testing toucan-js Sentry SDK, which transitively uses fetch to send requests to Sentry after calling captureMessage. miniflare, the test environment for the library, uses undici to polyfill fetch under the hood and provides its MockAgent via getMiniflareFetchMock() global.

@KhafraDev
Copy link
Member

KhafraDev commented Nov 6, 2022

Miniflare creates a wrapper around a Dispatcher (called MiniflareDispatcher) where its inner property is a MockAgent (for this example). Due to this, the check here is failing and defaulting to returning a web ReadableStream.

body: fetchParams.controller.dispatcher.isMockActive ? request.body && request.body.source : body,


This is an issue in Miniflare where the fix would (probably) be:

diff --git a/packages/core/src/standards/http.ts b/packages/core/src/standards/http.ts
index 31d9615..8074866 100644
--- a/packages/core/src/standards/http.ts
+++ b/packages/core/src/standards/http.ts
@@ -736,6 +736,11 @@ class MiniflareDispatcher extends Dispatcher {
     // @ts-expect-error just want to pass through to global dispatcher here
     return this.inner.destroy(...args);
   }
+
+  get isMockActive () {
+    // @ts-expect-error not all dispatchers may have an isMockActive property        
+    return this.inner.isMockActive
+  }
 }

 export async function fetch(

@robertcepa
Copy link
Author

Thank you for investigating @KhafraDev . I will take this up with the maintainers of miniflare.

My $.02: I think it would still be worthwhile to allow async functions in reply. This would allow devs to work around this kind of issue without getting blocked, and also allow using other async functions within the callback. Closing this and will open a new feature request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants