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

node-fetch version upgrade #698

Closed
wants to merge 1 commit into from
Closed

node-fetch version upgrade #698

wants to merge 1 commit into from

Conversation

Vhndaree
Copy link

Fixes #697

Copy link

@marck283 marck283 left a comment

Choose a reason for hiding this comment

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

NPM's node-fetch package versions prior to 3.2.10 are subject to both a MaxListenersExceededWarning: Possible EventEmitter memory leak detected warning and a Regular Expression Denial of Service bug, so I think that the node-fetch dependency should be upgraded to node-fetch@3.2.10 instead of node-fetch@2.7.0.

- According to https://security.snyk.io/package/npm/node-fetch, there
  are some security vulnerabilities in the package and the vulnerability
  is fixed on >v2.7.0
@Vhndaree
Copy link
Author

Vhndaree commented Sep 29, 2023

Hi @marck283 Thanks, that is a great idea but
node-fetch changed to ES modules by default from v3 so if we want to stick with the similar pattern of require() then it is not possible at the moment. So, I am sticking with v2.7.

@marck283
Copy link

marck283 commented Sep 30, 2023

I understand your point but consider that solving the MaxListenersExceededWarning warning will lead to less memory usage. As shown in these test runs (taken from here):

  • without the patch:
memoryUsage.rss: 35.97 Mb ⇒ 96.75 Mb (Diff: 168.99%)
memoryUsage.heapTotal: 7.11 Mb ⇒ 49.87 Mb (Diff: 601.10%)
memoryUsage.heapUsed: 5.23 Mb ⇒ 29.92 Mb (Diff: 471.66%)
memoryUsage.external: 1.04 Mb ⇒ 16.76 Mb (Diff: 1512.99%)
memoryUsage.arrayBuffers: 0.03 Mb ⇒ 15.68 Mb (Diff: 50570.52%)
  • with the patch:
memoryUsage.rss: 35.87 Mb ⇒ 47.92 Mb (Diff: 33.61%)
memoryUsage.heapTotal: 7.11 Mb ⇒ 7.62 Mb (Diff: 7.14%)
memoryUsage.heapUsed: 5.22 Mb ⇒ 6.86 Mb (Diff: 31.51%)
memoryUsage.external: 1.04 Mb ⇒ 8.10 Mb (Diff: 679.89%)
memoryUsage.arrayBuffers: 0.03 Mb ⇒ 0.21 Mb (Diff: 592.11%)

the memory usage cuts to half or more in all the tested cases.

When it comes to the require() pattern, Node.js users that run on version 13.2 and above can import the module by using the dynamic import instruction. This is why I think you should change the require() instructions to dynamic imports, so users that run on versions of Node.js below 13.2 should be able to import this module using the standard require(). Furthermore, by using the dynamic import instructions you can upgrade the node-fetch package this module runs on to its latest version. To allow the usage of the dynamic imports, however, you will have to change something in the files containing the TypeScript compilation settings.
For example, the file tsconfig.json should be modified as this (changing the file extension back to JSON): tsconfig.txt

@Vhndaree Vhndaree closed this Oct 23, 2023
@Vhndaree Vhndaree deleted the iss-697 branch October 23, 2023 09:04
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.

Security issue in node-fetch < 2.7.0
3 participants