TypeError in Node 12 #1509

panva · 2019-04-17T20:20:45Z

A quick heads up, feel free to close once you acknowledge. The test suite does not pass under node 12 (both nock latest and beta channels).

Tested using node nighly build, installed using

NVM_NODEJS_ORG_MIRROR=https://nodejs.org/download/nightly/
nvm i node

Here's the trace i'm getting when pre-testing my modules' test suite relying on nock under node 12, same issue.

TypeError [ERR_INVALID_ARG_TYPE]: The first argument must be one of type string, Buffer, ArrayBuffer, Array, or Array-like Object. Received type function
    at Function.from (buffer.js:218:9)
    at OverriddenClientRequest.RequestOverrider.req.write (/node_modules/nock/lib/request_overrider.js:112:27)
    at OverriddenClientRequest.RequestOverrider.req.end (/node_modules/nock/lib/request_overrider.js:133:11)
    at handleRequest (/node_modules/got/source/request-as-event-emitter.js:211:14)
    at get (/node_modules/got/source/request-as-event-emitter.js:235:5)
    at Immediate.<anonymous> (/node_modules/got/source/request-as-event-emitter.js:306:10)

The text was updated successfully, but these errors were encountered:

paulmelnikow · 2019-04-17T20:51:24Z

Thanks for the report!

We'd be happy to have help looking into and addressing the root cause!

paulmelnikow · 2019-04-17T20:58:53Z

It looks like it has to do with the allowed arguments to request.end. We don't handle the case where callback is specified but buffer and encoding are not, or where buffer and callback are provided but encoding is not.

Not sure why this is different in Node 12, though. 🤔

It would be helpful to have some more tests written from the perspective of an http client library, which check that the functions can be called in all the same ways as the native functions.

panva · 2019-04-23T19:30:34Z

The "problem" is that Buffer.from now throws on invalid inputs.

V11

> Buffer.from(()=>{})
<Buffer >

V12

> Buffer.from(()=>{})
Thrown:
TypeError [ERR_INVALID_ARG_TYPE]: The first argument must be one of type string, Buffer, ArrayBuffer, Array, or Array-like Object. Received type function
    at Function.from (buffer.js:219:9)
    at repl:1:8

request_overrider's req.write should handle the "optionality" of arguments and not blindly pass them to Buffer.from

panva · 2019-04-24T09:01:29Z

I am now applying this patch to nock 10.0.6 to have my CI pass under node 12. It's probably not the complete solution tho.

gr2m · 2019-04-24T16:35:55Z

@panva meta question: how do the patches get applied in https://github.com/panva/node-oidc-provider?

panva · 2019-04-24T16:53:01Z

@gr2m travis.yml before_script hook using patch-package module.

mastermatt · 2019-04-30T21:42:47Z

For reference, here is how Node handles the optional params for end.
https://github.com/nodejs/node/blob/master/lib/_http_outgoing.js#L672-L678

Personally, I prefer @panva version because Node allows for .end(callback, encoding) but the docs don't cover it.

paulmelnikow · 2019-05-01T16:40:04Z

This issue feels important to resolve! ♨️

For reference, here is how Node handles the optional params for end.
https://github.com/nodejs/node/blob/master/lib/_http_outgoing.js#L672-L678

Personally, I prefer @panva version because Node allows for .end(callback, encoding) but the docs don't cover it.

We should definitely match Node's behavior and the docs. If Node's behavior and the docs conflict we can match the behavior, though we obviously should not implement any new behavior.

This should be tested with a test that uses http, calls request.end() in the various expected ways, and asserts the result is as intended.

In general I'd like to segregate the tests that test nock's mock surface, i.e. the part that HTTP clients interact with, from the high-level functional tests calling through the nock public interface, that make up most of our tests. The new tests for this issue could go in a new file and I can work on collecting the rest of these tests at a later date.

If anyone wants to pick this up, I'd be happy to review it!

According to docs, `req.end` can accept callback as a first argument. That's what `got` module does. Fixes nock#1509 ``` request.end([data[, encoding]][, callback])# History data <string> | <Buffer> encoding <string> callback <Function> Returns: <this> Finishes sending the request. If any parts of the body are unsent, it will flush them to the stream. If the request is chunked, this will send the terminating '0\r\n\r\n'. If data is specified, it is equivalent to calling request.write(data, encoding) followed by request.end(callback). If callback is specified, it will be called when the request stream is finished. ```

According to the docs, `req.end` can accept callback as a first argument. That's what `got` module does. Closes #1509 ``` request.end([data[, encoding]][, callback])# History data <string> | <Buffer> encoding <string> callback <Function> Returns: <this> Finishes sending the request. If any parts of the body are unsent, it will flush them to the stream. If the request is chunked, this will send the terminating '0\r\n\r\n'. If data is specified, it is equivalent to calling request.write(data, encoding) followed by request.end(callback). If callback is specified, it will be called when the request stream is finished. ```

nockbot · 2019-05-09T13:27:02Z

🎉 This issue has been resolved in version 11.0.0-beta.13 🎉

The release is available on:

Your semantic-release bot 📦🚀

panva · 2019-05-09T13:40:55Z

@paulmelnikow would it be too much of a hassle to land this on the 10.x release line as well?

paulmelnikow · 2019-05-09T13:53:06Z

11.x will be released very soon and feedback on the beta releases would be very helpful. Though if someone wants to backport the fix and the test in a new PR on the master branch, I suppose we could ship it!

gugu · 2019-05-09T13:54:08Z

Thanks, I upgraded to 11.0.0-beta.13, works fine for me

paulmelnikow · 2019-05-09T13:58:11Z

Thanks @gugu for implementing this fix!

I think we still need to handle the .end(encoding, callback) form. I'll open a new issue for that.

Added: #1549

gugu · 2019-05-09T14:25:55Z

Do we? According to the docs, signature is
request.end([data[, encoding]][, callback])
it means, it can be
request.end(data, encoding, callback) or
request.end(data, encoding) or
request.end(callback) or
request.end(data, callback) or
request.end(data) or
request.end()

According to this, .end(encoding, callback) is not a valid signature

https://nodejs.org/api/http.html#http_request_end_data_encoding_callback

mastermatt · 2019-05-09T14:30:56Z

@gugu check out my comment above, #1509 (comment)
The docs don't cover that form, but the implementation does. The question is which way do we prefer?

Since we're trying to mimic, it might be worth simply copying the implementation from the Node source, despite the docs. 🤷‍♂

paulmelnikow · 2019-05-09T14:30:56Z

Ah, indeed!

Though I think we're not handling request.end(data, callback) case.

Added: I think matching the undocumented behavior of the implementation is a good idea.

panva · 2019-05-09T17:17:48Z

@gugu thanks for taking the time to fix this!

* nock@10.0.6 -> nock@11.0.0-beta.13 Addressed this bug: nock/nock#1509

nockbot · 2019-08-13T00:07:52Z

🎉 This issue has been resolved in version 11.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

According to the docs, `req.end` can accept callback as a first argument. That's what `got` module does. Closes #1509 ``` request.end([data[, encoding]][, callback])# History data <string> | <Buffer> encoding <string> callback <Function> Returns: <this> Finishes sending the request. If any parts of the body are unsent, it will flush them to the stream. If the request is chunked, this will send the terminating '0\r\n\r\n'. If data is specified, it is equivalent to calling request.write(data, encoding) followed by request.end(callback). If callback is specified, it will be called when the request stream is finished. ```

paulmelnikow added pinned bug labels Apr 17, 2019

gr2m added the pull request welcome label Apr 17, 2019

gugu mentioned this issue May 8, 2019

Support req.end with callback only #1547

Merged

paulmelnikow closed this as completed in #1547 May 9, 2019

nockbot added the released label May 9, 2019

paulmelnikow changed the title ~~issues under node 12~~ TypeError in Node 12 May 9, 2019

paulmelnikow mentioned this issue May 9, 2019

.end(data, callback) doesn't work; .end(callback) doesn't work when recording #1549

Closed

paulmelnikow mentioned this issue May 9, 2019

fix: Fix req.end(cb) when recording in Node 12 #1551

Merged

weekly-digest bot mentioned this issue May 12, 2019

Weekly Digest (5 May, 2019 - 12 May, 2019) #1554

Closed

sindresorhus mentioned this issue Jun 3, 2019

Got.js throws ERR_INVALID_ARG_TYPE when mocking with nock on Node 12.x sindresorhus/got#817

Closed

2 tasks

sounisi5011 added a commit to sounisi5011/metalsmith-netlify-published-date that referenced this issue Jul 20, 2019

⬆️ Upgrade dependency: nock

0bf38df

* nock@10.0.6 -> nock@11.0.0-beta.13 Addressed this bug: nock/nock#1509

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TypeError in Node 12 #1509

TypeError in Node 12 #1509

panva commented Apr 17, 2019 •

edited

paulmelnikow commented Apr 17, 2019

paulmelnikow commented Apr 17, 2019

panva commented Apr 23, 2019 •

edited

panva commented Apr 24, 2019

gr2m commented Apr 24, 2019

panva commented Apr 24, 2019

mastermatt commented Apr 30, 2019

paulmelnikow commented May 1, 2019

nockbot commented May 9, 2019

panva commented May 9, 2019

paulmelnikow commented May 9, 2019

gugu commented May 9, 2019

paulmelnikow commented May 9, 2019 •

edited

gugu commented May 9, 2019 •

edited

mastermatt commented May 9, 2019

paulmelnikow commented May 9, 2019 •

edited

panva commented May 9, 2019

nockbot commented Aug 13, 2019

TypeError in Node 12 #1509

TypeError in Node 12 #1509

Comments

panva commented Apr 17, 2019 • edited

paulmelnikow commented Apr 17, 2019

paulmelnikow commented Apr 17, 2019

panva commented Apr 23, 2019 • edited

panva commented Apr 24, 2019

gr2m commented Apr 24, 2019

panva commented Apr 24, 2019

mastermatt commented Apr 30, 2019

paulmelnikow commented May 1, 2019

nockbot commented May 9, 2019

panva commented May 9, 2019

paulmelnikow commented May 9, 2019

gugu commented May 9, 2019

paulmelnikow commented May 9, 2019 • edited

gugu commented May 9, 2019 • edited

mastermatt commented May 9, 2019

paulmelnikow commented May 9, 2019 • edited

panva commented May 9, 2019

nockbot commented Aug 13, 2019

panva commented Apr 17, 2019 •

edited

panva commented Apr 23, 2019 •

edited

paulmelnikow commented May 9, 2019 •

edited

gugu commented May 9, 2019 •

edited

paulmelnikow commented May 9, 2019 •

edited