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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 3 commits into from
Aug 11, 2023
Merged

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

merged 3 commits into from
Aug 11, 2023

Conversation

KeesCBakker
Copy link
Collaborator

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: nock/nock#2397 (comment)

@KeesCBakker
feat: remove obsolete request package, add fetch
2a8d151
@KeesCBakker
chore: convert download to fetch
4d5bb06
@KeesCBakker
fix: make fetch behave when testing.
29483cf 
When testing the nock package does not know how to behave with the native fetch, so we use 'node-fetch' to make fetch testable with nock. More info: nock/nock#2397 (comment)
@KeesCBakker
Copy link
Collaborator Author

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

image

New package:

image

@KeesCBakker
Copy link
Collaborator Author

@stephenyeargin, any thoughts on this?

stephenyeargin
stephenyeargin reviewed Aug 9, 2023
View reviewed changes
Copy link
Owner

@stephenyeargin stephenyeargin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

.gitignore Show resolved Hide resolved
package.json
@@ -42,7 +42,7 @@
"main": "index.js",
"scripts": {
"release": "release-it",
"test": "mocha \"test/**/*.js\" --reporter spec",
"test": "mocha \"test/**/*.js\" --reporter spec --no-experiemental-fetch",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing I don't know is if this flag is necessary for the normal operation of the script package. If so, we'd run into a bit of trouble for folks running < v18.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently testing with Docker if we can get away with this one:

# Define NODE_VERSION as a build argument immediately before the FROM statement
ARG NODE_VERSION=20
FROM node:$NODE_VERSION 

WORKDIR /build

# Copy package files and install dependencies
COPY package.json package-lock.json ./
RUN npm install

COPY index.js ./
COPY script ./script
COPY src ./src
COPY test ./test

# Run tests
RUN npm test

And the script that fires it off:

$nodeVersions = @('8', '9', '10', '12', '14', '18', '20')

foreach ($version in $nodeVersions) {
    Write-Output "Building Docker image with NODE_VERSION $version"
    docker build . --build-arg "NODE_VERSION=$version"

    # Check if docker build had an error
    if ($LASTEXITCODE -ne 0) {
        Write-Output "Error encountered. Stopping script."
        exit $LASTEXITCODE
    }
}

Write-Output "Build process completed."

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Node 8 does not work:

#0 0.898 Error: yargs parser supports a minimum Node.js version of 10. Read our version support policy: https://github.com/yargs/yargs-parser#supported-nodejs-versions

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same error on Node 9 ;-)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Node 10 fails on Mocha:

#0 0.649
#0 0.649 > hubot-grafana@4.0.0 test /build
#0 0.649 > mocha "test/**/*.js" --reporter spec --no-experiemental-fetch
#0 0.649
#0 0.837
#0 0.837 Error: Not supported
#0 0.837     at Object.exports.doImport (/build/node_modules/mocha/lib/nodejs/esm-utils.js:35:41)
#0 0.837     at formattedImport (/build/node_modules/mocha/lib/nodejs/esm-utils.js:9:28)
#0 0.837     at Object.exports.requireOrImport (/build/node_modules/mocha/lib/nodejs/esm-utils.js:42:34)
#0 0.837     at Object.exports.loadFilesAsync (/build/node_modules/mocha/lib/nodejs/esm-utils.js:100:34)
#0 0.837     at Mocha.loadFilesAsync (/build/node_modules/mocha/lib/mocha.js:447:19)
#0 0.837     at singleRun (/build/node_modules/mocha/lib/cli/run-helpers.js:125:15)
#0 0.837     at exports.runMocha (/build/node_modules/mocha/lib/cli/run-helpers.js:190:10)
#0 0.837     at Object.exports.handler (/build/node_modules/mocha/lib/cli/run.js:370:11)
#0 0.837     at innerArgv.then.argv (/build/node_modules/yargs/build/index.cjs:443:71)
#0 0.837     at process._tickCallback (internal/process/next_tick.js:68:7)
#0 0.837     at Function.Module.runMain (internal/modules/cjs/loader.js:834:11)
#0 0.837     at startup (internal/bootstrap/node.js:283:19)
#0 0.837     at bootstrapNodeJSCore (internal/bootstrap/node.js:623:3)
#0 0.843 npm ERR! Test failed.  See above for more details.```

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turned it around for faster results: $nodeVersions = @('20', '18', '16', '14', '12')

Versions 20, 18 and 16 work

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Version 14 crashes on:

#0 1.122
#0 1.122 > hubot-grafana@4.0.0 test /build
#0 1.122 > mocha "test/**/*.js" --reporter spec --no-experiemental-fetch
#0 1.122
#0 1.765
#0 1.768
#0 1.771   rocketchat
#0 1.771     rocketchat upload
#0 2.166 {"level":50,"time":1691648853747,"pid":19,"hostname":"buildkitsandbox","name":true,"msg":"Unable to load /build/src/grafana: TypeError: this.name.replace is not a function\n    at MockRobot.respondPattern (/build/node_modules/hubot/src/robot.js:137:28)\n    at MockRobot.respond (/build/node_modules/hubot/src/robot.js:122:20)\n    at module.exports (/build/src/grafana.js:106:9)\n    at MockRobot.loadFile (/build/node_modules/hubot/src/robot.js:368:9)\n    at Helper.createRoom (/build/node_modules/hubot-test-helper/src/index.js:155:15)\n    at Context.<anonymous> (/build/test/grafana-rocketchat-test.js:55:21)\n    at callFn (/build/node_modules/mocha/lib/runnable.js:366:21)\n    at Hook.Runnable.run (/build/node_modules/mocha/lib/runnable.js:354:5)\n    at next (/build/node_modules/mocha/lib/runner.js:498:10)\n    at Immediate._onImmediate (/build/node_modules/mocha/lib/runner.js:559:5)\n    at processImmediate (internal/timers.js:464:21)"}
#0 2.180 npm ERR! Test failed.  See above for more details.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Version 12 crashes on a null coalescing operator:

 > [9/9] RUN npm test:
#0 0.883
#0 0.883 > hubot-grafana@4.0.0 test /build
#0 0.883 > mocha "test/**/*.js" --reporter spec --no-experiemental-fetch
#0 0.883
#0 1.300
#0 1.300 /build/node_modules/hubot/src/robot.js:70
#0 1.300     this.adapterName = adapter ?? 'shell'
#0 1.300                                 ^
#0 1.300
#0 1.300 SyntaxError: Unexpected token '?'
#0 1.300     at wrapSafe (internal/modules/cjs/loader.js:915:16)
#0 1.300     at Module._compile (internal/modules/cjs/loader.js:963:27)
#0 1.300     at Object.Module._extensions..js (internal/modules/cjs/loader.js:1027:10)
#0 1.300     at Module.load (internal/modules/cjs/loader.js:863:32)
#0 1.300     at Function.Module._load (internal/modules/cjs/loader.js:708:14)
#0 1.300     at Module.require (internal/modules/cjs/loader.js:887:19)
#0 1.300     at require (internal/modules/cjs/helpers.js:74:18)
#0 1.300     at Object.<anonymous> (/build/node_modules/hubot/es2015.js:5:15)
#0 1.300     at Module._compile (internal/modules/cjs/loader.js:999:30)
#0 1.300     at Object.Module._extensions..js (internal/modules/cjs/loader.js:1027:10)
#0 1.300     at Module.load (internal/modules/cjs/loader.js:863:32)
#0 1.300     at Function.Module._load (internal/modules/cjs/loader.js:708:14)
#0 1.300     at Module.require (internal/modules/cjs/loader.js:887:19)
#0 1.300     at require (internal/modules/cjs/helpers.js:74:18)
#0 1.300     at Object.<anonymous> (/build/node_modules/hubot-test-helper/src/index.js:5:15)
#0 1.300     at Module._compile (internal/modules/cjs/loader.js:999:30)
#0 1.300     at Object.Module._extensions..js (internal/modules/cjs/loader.js:1027:10)
#0 1.300     at Module.load (internal/modules/cjs/loader.js:863:32)
#0 1.300     at Function.Module._load (internal/modules/cjs/loader.js:708:14)
#0 1.300     at Module.require (internal/modules/cjs/loader.js:887:19)
#0 1.300     at require (internal/modules/cjs/helpers.js:74:18)
#0 1.300     at Object.<anonymous> (/build/test/grafana-rocketchat-test.js:1:16)
#0 1.300     at Module._compile (internal/modules/cjs/loader.js:999:30)
#0 1.300     at Object.Module._extensions..js (internal/modules/cjs/loader.js:1027:10)
#0 1.300     at Module.load (internal/modules/cjs/loader.js:863:32)
#0 1.300     at Function.Module._load (internal/modules/cjs/loader.js:708:14)
#0 1.300     at ModuleWrap.<anonymous> (internal/modules/esm/translators.js:195:29)
#0 1.300     at ModuleJob.run (internal/modules/esm/module_job.js:145:37)
#0 1.300     at async Loader.import (internal/modules/esm/loader.js:182:24)
#0 1.300     at async formattedImport (/build/node_modules/mocha/lib/nodejs/esm-utils.js:9:14)
#0 1.300     at async Object.exports.requireOrImport (/build/node_modules/mocha/lib/nodejs/esm-utils.js:42:28)
#0 1.300     at async Object.exports.loadFilesAsync (/build/node_modules/mocha/lib/nodejs/esm-utils.js:100:20)
#0 1.300     at async singleRun (/build/node_modules/mocha/lib/cli/run-helpers.js:125:3)
#0 1.300     at async Object.exports.handler (/build/node_modules/mocha/lib/cli/run.js:370:5)
#0 1.310 npm ERR! Test failed.  See above for more details.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The flag itself is not a problem for older Node versions.

@KeesCBakker
Copy link
Collaborator Author

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
Copy link
Collaborator Author

KeesCBakker commented Aug 10, 2023 •
edited

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
Copy link
Collaborator Author

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.

@stephenyeargin stephenyeargin merged commit eab2553 into stephenyeargin:main Aug 11, 2023
@stephenyeargin stephenyeargin mentioned this pull request Aug 12, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stephenyeargin stephenyeargin stephenyeargin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Getting rid of the obsolete request dependency
2 participants
@KeesCBakker @stephenyeargin