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

Fix bug when set agent for needle & add test cases #54

Merged
merged 1 commit into from Apr 14, 2021

Conversation

taina0407
Copy link

Using deepmerge make agent object invalid with node http, using lodash will avoid this issue.
Also added test case for using agent

@puzrin
Copy link
Member

puzrin commented Apr 12, 2021

I would not like switch to lodash.merge. Could you explain details about problem (and minimal example how to replroduce)?

@taina0407
Copy link
Author

taina0407 commented Apr 14, 2021

I would not like switch to lodash.merge. Could you explain details about problem (and minimal example how to replroduce)?

As I comment above, because the "agent" configuration instance will be convert to pure object when using deepmerge library, that will cause the needle plugin to fail when using agent (for example to keep alive TCP connection)

Here is the simple code for procedure this error:

const http = require("http");
const https = require("https");
const Probe = require("probe-image-size");

(async () => {
  try {
    await Probe("http://via.placeholder.com/350x150", {
      agent: new http.Agent({
        keepAlive: true,
      }),
    });
  } catch (error) {
    console.error(error);
  }

  try {
    await Probe("https://via.placeholder.com/350x150", {
      agent: new https.Agent({
        keepAlive: true,
      }),
    });
  } catch (error) {
    console.error(error);
  }
})();

These will throw exception (Tested with Node 12 and 14)

TypeError [ERR_INVALID_ARG_TYPE]: The "options.agent" property must be one of Agent-like Object, undefined, or false. Received an instance of Object
    at new ClientRequest (_http_client.js:133:11)
    at Object.request (http.js:46:10)
    at Needle.send_request (/FAKE_PATH/server/node_modules/needle/lib/needle.js:504:26)
    at next (/FAKE_PATH/server/node_modules/needle/lib/needle.js:391:10)
    at Needle.start (/FAKE_PATH/server/node_modules/needle/lib/needle.js:394:17)
    at Function.module.exports.<computed> [as get] (/FAKE_PATH/server/node_modules/needle/lib/needle.js:817:61)
    at /FAKE_PATH/server/node_modules/probe-image-size/http.js:31:23
    at new Promise (<anonymous>)
    at probeHttp (/FAKE_PATH/server/node_modules/probe-image-size/http.js:27:10)
    at get_image_size (/FAKE_PATH/server/node_modules/probe-image-size/index.js:17:10) {
  code: 'ERR_INVALID_ARG_TYPE'
}

@puzrin
Copy link
Member

puzrin commented Apr 14, 2021

Got it. Seems deepmerge has desired changes in upcoming 5.x, but development is not active and we can't wait without deadlines.

Let's accept your proposal, and [may be] roll back later. Thanks for your efforts!

@puzrin puzrin merged commit 649af35 into nodeca:master Apr 14, 2021
@puzrin
Copy link
Member

puzrin commented Apr 14, 2021

Published 7.1.0

puzrin pushed a commit to nodeca/url-unshort that referenced this pull request Dec 1, 2021
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.

None yet

2 participants