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

remove rimraf dependency #720

Merged
merged 1 commit into from
Jul 4, 2024
Merged

remove rimraf dependency #720

merged 1 commit into from
Jul 4, 2024

Conversation

benmccann
Copy link
Contributor

@benmccann benmccann commented Jun 27, 2024

closes #692
closes #707

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

Copy link
Contributor

@pnappa pnappa left a comment

Choose a reason for hiding this comment

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

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

However, I believe some code needs fixing.

lib/clean.js Outdated Show resolved Hide resolved
lib/util/napi.js Outdated Show resolved Hide resolved
lib/util/napi.js Outdated Show resolved Hide resolved
@pnappa
Copy link
Contributor

pnappa commented Jun 27, 2024

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

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

14.14.0

lib/util/napi.js Outdated Show resolved Hide resolved
lib/util/napi.js Outdated Show resolved Hide resolved
@cclauss
Copy link
Collaborator

cclauss commented Jun 29, 2024

Please resolve git conflicts.

lib/clean.js Show resolved Hide resolved
@benmccann benmccann force-pushed the rimraf branch 2 times, most recently from aaea73e to 7d5cc1a Compare June 29, 2024 15:21
@cclauss cclauss requested a review from pnappa June 29, 2024 15:42
@cclauss
Copy link
Collaborator

cclauss commented Jun 29, 2024

@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
Copy link
Contributor

ronilan commented Jun 29, 2024

@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.

lib/clean.js Show resolved Hide resolved
lib/util/napi.js Show resolved Hide resolved
lib/util/napi.js Show resolved Hide resolved
@pnappa
Copy link
Contributor

pnappa commented Jul 1, 2024

@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
Copy link
Contributor Author

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

@cclauss
Copy link
Collaborator

cclauss commented Jul 3, 2024

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.

@cclauss
Copy link
Collaborator

cclauss commented Jul 3, 2024

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

benmccann commented Jul 3, 2024

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

cclauss commented Jul 3, 2024

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

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
Copy link
Contributor

pnappa commented Jul 4, 2024

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

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.

Copy link
Collaborator

@cclauss cclauss left a comment

Choose a reason for hiding this comment

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

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

@cclauss cclauss merged commit e882b7e into mapbox:master Jul 4, 2024
12 checks passed
@benmccann benmccann deleted the rimraf branch July 4, 2024 03:49
@43081j
Copy link

43081j commented Aug 5, 2024

@cclauss any chance we can get a release of this? 👀

@benmccann
Copy link
Contributor Author

The latest release was 1.1.0-dev.1, but this PR was not included in that release. Unfortunately releasing this PR by itself won't help a ton as minizlib also pulls in rimraf. That PR will need to be merged as well to see the benefit.

It's probably worth trying to get some other PRs in as well before doing a new release. @cclauss is there anything I can do to help on that front? I think several PRs in the queue are ready to go

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.

replace rimraf with native rm
6 participants