clearbit / clearbit-node

Node library for querying the Clearbit business intelligence APIs
https://clearbit.com/docs
MIT License
69 stars 35 forks source link

Fix dependency clearbit/needle breaking npm 7 builds without git/ssh (fix #45, fix #49) #50

Closed ronjouch closed 2 years ago

ronjouch commented 3 years ago

This will solve git/ssh-less CI builds failing (#45, #49) with:

npm ERR! Error while executing:
npm ERR! /usr/bin/git ls-remote -h -t ssh://git@github.com/clearbit/needle.git

This happens because needle is specified in clearbit-node's package.json as https://github.com/clearbit/clearbit-node/blob/ee04e8475fa0461fc88d31eed96bc4f0989475fb/package.json#L28

, and this is unusable in CI machines that don't have git or ssh. But this clearbit/needle#84d28b5f is needle 0.7.1, plus two patches sitting on top of it. Are these patches still required?

1. First patch: require(package.json)

As described in https://github.com/clearbit/clearbit-node/pull/37 ,

In #36 @andymjames mentioned that the way the version of Needle we use has a way of requiring the version that is incompatible with some tools.

So, clearbit forked needle and patched the JSON.parse(fs.readFileSync(...package.json)) into a require(package.json); https://github.com/clearbit/needle/commit/84d28b5f2c3916db1e7eb84aeaa9d976cc40054b

Thankfully, this fork & patch is no longer required since latest needle does the proper require() call 🙂. See the latest upstream needle code at https://github.com/tomas/needle/blob/master/lib/needle.js around line 24:

var version     = require('../package.json').version;

2. Second patch: stringifyArray

https://github.com/clearbit/needle/commit/ea566981076fb1f3864e4d736f522831364333ae patched needle's stringifyArray that is also the current upstream behavior: https://github.com/tomas/needle/blob/master/lib/querystring.js#L29

  for (var i = 0, len = arr.length; i < len; i++) {
    if (prefix)
      ret.push(stringify(arr[i], prefix + '[]')); // <-- what used to be patched, to the same thing
    else
      ret.push(stringify(arr[i]));
  }

→ The fork is no longer necessary. This PR replaces usage of the fork and comes back to the public npm version, which will no longer be problematic in CI builds.

If this PR is accepted and merged, please please don't come back to such forking & patching, it is problematic:

  1. In CI, where ssh/git may not be available
  2. On npm behavior changes, see https://github.com/npm/cli/issues/2610
  3. Security-wise
ronjouch commented 3 years ago

@davidlumley does that make sense? If merging this, please make a release for it :pray: .

AdelUnito commented 3 years ago

In need for this Solution as well, can you please merge it

t0mstah commented 3 years ago

Please merge @davidlumley @maccman @bendrucker @robholland @dcadenas @tristandunn @gregors

ronjouch commented 2 years ago

FYI everybody, official message (emphasis mine) from Clearbit's head of support after following up with them every 2 months.

Hi Ronan,

Thanks for reaching out again. To best serve our wide customer base, we continue to make regular updates to our product, improving breadth, depth, and accuracy of our data. We follow industry standard prioritization of engineering resources based on impact and urgency. We are not able to dedicate resources to the node.js library.

Best,

<Name censored, I don't want to name names> | Head of Customer Support | Clearbit

In summary: don't use this library and make REST calls on your own. Which is acceptable, but why pretend to paying customers you're maintaining a library, then?

Follow-up issue: https://github.com/clearbit/clearbit-node/issues/54