search

stephenyeargin / hubot-grafana

📈🤖 Query Grafana dashboards
http://docs.grafana.org/tutorials/hubot_howto/
MIT License
154 stars 48 forks source link

feat: swap out obsolete request package for node-fetch #162

Closed KeesCBakker closed 1 year ago

KeesCBakker commented 1 year ago

Should fix #161. Tried to keep the changes to a minimum. Did some local testing and my Grafana/Slack/S3 setup works like a charm.

Tests all work, but I'm not sure if they would have ever hit the actual request lib.

The only drawback I see is that you need to add --no-experiemental-fetch when testing. This has to do with the nock lib: https://github.com/nock/nock/issues/2397#issuecomment-1568646137

KeesCBakker commented 1 year ago

This will also remove some of the warnings when auditing the package. Current package will return:

image

New package:

image

KeesCBakker commented 1 year ago

@stephenyeargin, any thoughts on this?

KeesCBakker commented 1 year ago

Minor comments. I had a branch that replaced request with axios, but yours here is less obtrusive.

I used to be big on Axios, but I swapped it all out for fetch (and node-fetch). In future versions it will become standard in Node. I like this site as well: https://danlevy.net/you-may-not-need-axios/

KeesCBakker commented 1 year ago

Conclusion:

20, 18, 16: work like a charm 14: MockBot fails somewhere? 12: Crash on null coalescing operator 10: Mocha fails Below 10: yargs parser not supported

KeesCBakker commented 1 year ago

I get the feeling that the main problem is the testing framework itself. And it makes total sense that Mocha for example has no support for old versions. Node fetch@3 says something about a minimal node version, but https://www.npmjs.com/package/node-fetch/v/2.6.7 (v2) says nothing. We use v2 because v3 is mjs.