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

req.write should throw error where chunk is undefined #458

Open
Tracked by #2517
mikicho opened this issue Sep 29, 2023 · 3 comments · May be fixed by #493 or #515
Open
Tracked by #2517

req.write should throw error where chunk is undefined #458

mikicho opened this issue Sep 29, 2023 · 3 comments · May be fixed by #493 or #515

Comments

@mikicho
Copy link
Contributor

mikicho commented Sep 29, 2023

https://github.com/mswjs/interceptors/blob/main/src/interceptors/ClientRequest/NodeClientRequest.ts#L101

chunk must not be undefined and throw:

TypeError [ERR_INVALID_ARG_TYPE]: The "chunk" argument must be of type string or an instance of Buffer or Uint8Array. Received undefined

I can open a PR, but because you ignored this case positively, I want to ensure I don't miss something.

About the solution, we can either:

  1. call to super.write() and let Node emit the error
  2. Return a custom error
@kettanaito
Copy link
Member

Hi, @mikicho. This is a good suggestion.

Counting on super.write() won't work because I'm sure Node will throw something connection-related if the socket connection failed (think of the mocked requests to non-existing hosts) and you try to write onto that socket.

I hate to re-create internal Node errors so let's brainstorm how we can achieve this behavior without repeating Node.

@mikicho
Copy link
Contributor Author

mikicho commented Dec 21, 2023

@kettanaito It seems like the input validation is the first thing Node does before trying to write into the socket (source) so we can do something like:

class NodeClientRequest {
  ...
  write(...args) {
     ...
    const [chunk, encoding, callback] = normalizeClientRequestWriteArgs(args)
    if (!chunk) {
      super.write()
    }
  }
}

This test passes:

it('does not allow empty chunk', async () => {
  const emitter = new Emitter<HttpRequestEventMap>()
  const request = new NodeClientRequest(
    normalizeClientRequestArgs('http:', httpServer.http.url('/comment'), {
      method: 'POST',
    }),
    {
      emitter,
      logger,
    }
  )

  // @ts-expect-error - test undefined chunk
  expect(() => request.write()).toThrow('The "chunk" argument must be of type string or an instance of Buffer or Uint8Array. Received undefined')
  // @ts-expect-error - test null chunk
  expect(() => request.write(null)).toThrow('May not write null values to stream')
})

@mikicho mikicho linked a pull request Dec 21, 2023 that will close this issue
@mikicho
Copy link
Contributor Author

mikicho commented Dec 21, 2023

I don't want to forget what I did here (or that I did lol), so I opened a PR: #493

Happy holidays :)

@kettanaito kettanaito linked a pull request Mar 12, 2024 that will close this issue
17 tasks
@kettanaito kettanaito added this to the Nock compatibility milestone Mar 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants