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

Issue-3 update dependencies #4

Merged
merged 1 commit into from
Sep 23, 2021
Merged

Conversation

Ghazgkull
Copy link

Addresses #3

This PR updates the dependencies of the @adnanrahic/node-gcstats module to the latest. Most importantly, it updates this package to stop using the deprecated node-pre-gyp package in favor of the supported @mapbox/node-pre-gyp module. See the deprecation notice on the old module here: https://www.npmjs.com/package/node-pre-gyp

This PR also includes updates to keep the unit tests working with the latest mocha version. I renamed the tests folder to test because that's what the newest Mocha expects. And I updated the test command to pass the --expose-gc command to node directly, since Mocha no longer supports passing the flag through.

@adnanrahic Please take a look.

Signed-off-by: Jared Burns <[email protected]>
@JNPLZ
Copy link

JNPLZ commented Sep 21, 2021

Awesome, @Ghazgkull !

@adnanrahic can you please take a look at this MR? Thanks!

@Jarco-dev
Copy link

Waiting on this, hope it can get merged soon.
(Due to the audit failing i can't install on linux atm)

@adnanrahic adnanrahic self-assigned this Sep 23, 2021
@adnanrahic adnanrahic merged commit 3069c80 into adnanrahic:master Sep 23, 2021
@baconandon
Copy link

baconandon commented Sep 29, 2021

Sorry to be the bearer of bad news, but... even though the node-pre-gyp dependency was removed it is still bundled

Which means

  1. npm i @sematext/gc-stats installs node-pre-gyp as well as @mapbox/node-pre-gyp
  2. npm audit => 😢 (because of tar dependencies from node-pre-gyp)

FWIW npm ls node-pre-gyp after the install shows

┬ @sematext/[email protected]
  └── [email protected]

(I would have added this as a comment on issue #3 but it is closed 🤷)

HTH

@Ghazgkull
Copy link
Author

@baconandon At this point, you know as much about this repo as I do. :-) I see that @adnanrahic created Issue #1 a while back to remove the bundled dependency, but it seems like that never happened. Is it as simple as just removing the bundled dependency lines from the package.json manifest or are there other things to consider?

@Ghazgkull
Copy link
Author

@baconandon @adnanrahic Actually, I decided to just give it a shot. I fixed everything up and submitted a PR: #6

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.

5 participants