remove rimraf dependency

[benmccann]

closes #692
closes #707

this removes quite a few dependencies, many of which are deprecated: https://npmgraph.js.org/?q=rimraf@5.0.5

[pnappa]

I prefer this PR to mine (#707), as removing dependencies is better than updating them.

However, I believe some code needs fixing.

[pnappa]

Also, for the same reasons as in my PR, this will be a breaking change (hopefully no-one's actually affected by it), as it drops support for old versions of node. See: #707 (comment)

Minimum required version is now 14.x (not sure which version specifically)

[benmccann]

Minimum required version is now 14.x (not sure which version specifically)

14.14.0

[cclauss]

Please resolve git conflicts.

[cclauss]

@ronilan @pnappa @acalcutt @benmccann Can you please help me by reviewing the open PRs on this repo?

When you think a pull request is useful and is ready to be merged, please consider giving it a positive review.

If you think a pull request needs work or should not be merged, please consider giving it a negative review.

Every ✅ at the top right of this page gives project maintainers confidence that the proposed changes have been read through and deemed both useful and safe to merge into the codebase. Lots of 👍 and "what is the ETA?" comments are easier for maintainers to ignore than ✔️✔️✔️✔️✔️ from several different reviewers.

Similarly for ❌ on the other side -- needs work or should not be merged or undocumented breaking change.

Anyone can review a pull request on GitHub. To do so here:

1. Scroll to the top of this page.
2. Click the Files changed tab and read through each file carefully looking for potential issues.
3. Click the Review changes button.
4. Click Approve (or one of the other options) and add comments only if they have not already been stated in the PR.
5. Click Submit review so that your ✅ or ❌ will be added to the list.

[ronilan]

@ronilan @pnappa @acalcutt @benmccann Can you please help me by reviewing the open PRs on this repo?

@cclauss - sorry, I ran out of play time. My GitHub Sponsors page is here: https://github.com/sponsors/ronilan Good luck to all that treat legacy code maintenance a hobby rather than work.

[pnappa]

@ronilan @pnappa @acalcutt @benmccann Can you please help me by reviewing the open PRs on this repo?

Hi Christian, I'm also not available to review PRs, I only made my PRs & reviewed the related PRs as a professional courtesy to try and patch vulnerable packages in dependencies. I no longer depend on this package, and as such can't justify helping during work hours (in my spare time I try to avoid computers).

node-argon2 was the package that I used that depended on node-pre-gyp, but they migrated to a different, more upkept package (prebuildify). I've talked about it in this PR before #705 (comment) . I think community effort should be spent on consolidating onto a single product solution rather than limping along with finite resources spread too thin.

Good luck, and thanks for all the fish!

[benmccann]

@cclauss this one has two approvals, so I think it should be good to merge now

[cclauss]

I would rather progress on #739 before we make any removals of dependencies. We have made a ton of changes and our test coverage is imperfect. Let's give users a chance to kick the tires of our current state before we replace parts.

• Needed: A GitHub Action to package and publish a new version #739

[cclauss]

If you are interested in making contributions while we await progress on #739, please consider...

Updating https://github.com/mapbox/node-pre-gyp/blob/master/README.md to reflect current versions of Node.js and our changes to automated testing.

The repo could use the addition of https://pre-commit.com or https://typicode.github.io/husky to do fast linting, type checking, spell checking, etc. at commit time.

% codespell

./CHANGELOG.md:412: validing ==> validating
./CHANGELOG.md:503: depedencies ==> dependencies
./test/run.util.js:73: pre-pending ==> pretending

(The last one is an incorrect change.)

Or look thru the @dependabot generated PRs that fail and add comments to those PRs about why they fail and what changes we should consider to make them pass.

Or create a changelog PR that describes the changes made for our upcoming release.

Or help us to understand our test coverage like https://about.codecov.io/blog/getting-started-with-code-coverage-for-javascript or similar.

[benmccann]

Updating https://github.com/mapbox/node-pre-gyp/blob/master/README.md to reflect current versions of Node.js and our changes to automated testing.

I've updated the supported versions of Node as part of #833.

The repo could use the addition of https://pre-commit.com/ or https://typicode.github.io/husky to do fast linting, type checking, spell checking, etc. at commit time.

I would prefer not to add these. These types of checks are better on the CI in my opinion where they can be run asynchronously as opposed to interrupting the developer workflow.

look thru the https://github.com/dependabot generated PRs that fail and add comments to those PRs about why they fail and what changes we should consider to make them pass

A lot of this effort would be wasted if done at this stage. PRs like this one remove a lot of our transitive dependencies. Rather than figuring out why an upgrade somewhere down the tree won't succeed we should first merge the PRs that remove or change dependencies. Then we can go through and figure out how to upgrade what's left.

It should be noted that the vast majority of both merged and pending dependabot PRs have no impact on users. I've sent a PR to reduce the noise from dependabot: #834

Or create a changelog PR that describes the changes made for our upcoming release.

Done! #833

We have made a ton of changes. Let's give users a chance to kick the tires of our current state before we replace parts.

I'm not sure I really agree with this. As you can see in the changelog PR (#833), we've only updated supported node versions, updated a few dependencies, and made one bug fix. This PR is less than a dozen lines and just changes the method we call to delete a file. Including it in an initial release would simplify the changelog for users because rather than there being two entries stating "updated rimraf" and "removed rimraf" in two different releases we could condense it down simply to "removed rimraf".

[cclauss]

breaking change worries me on a codebase that is not being used... #720 (comment)

My years of effort on node-gyp encourages me to make small moves.

[benmccann]

The breaking change was introducing the engines field that requires Node 18. That's already been done in an earlier PR. The APIs used in this PR only require Node 14.14.0 and so don't introduce any new breaking changes.

[pnappa]

The breaking change was introducing the engines field that requires Node 18. That's already been done in an earlier PR. The APIs used in this PR only require Node 14.14.0 and so don't introduce any new breaking changes.

That's right. It's a matter of philosophy whether bumping the minimum required node version is a breaking change or not - I think it is, as a user upgrading a package following semver schemes may have their app fail to compile as a result. However, we don't care about them in this scenario, as they're using EOL'd versions of NodeJS, and have much bigger problems to consider (unpatched security vulnerabilities et al).

Others disagree whether it's a breaking change or not - here, no functional run-time behaviour has changed, and as such should be fine to merge.

[benmccann]

I agree that bumping the minimum node version is a breaking change, but my point is that's not being done here because we've earlier specified Node 18 as the minimum.

[cclauss]

Thanks for making it clear. Let's go...