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

Sync deps and engines with npm #2770

Merged
merged 8 commits into from Jun 20, 2023
Merged

Sync deps and engines with npm #2770

merged 8 commits into from Jun 20, 2023

Conversation

lukekarrys
Copy link
Member

@lukekarrys lukekarrys commented Dec 9, 2022

Checklist
Description of change

The main purpose of this PR is to update dependencies in order to dedupe as many deps as possible within npm. As can be seen in the npm/cli source there are a number of dependencies that end up nested within node-gyp/node_modules due to conflicting versions.

To accomplish this a number of other changes were made:

  • fe5b13a BREAKING CHANGE update engines.node to ^14.17.0 || ^16.13.0 || >=18.0.0
  • a729c15 Update standard to work with newer JS syntax and fix new lint errors
  • 9ccc66d The @npm/cli-team team will be deprecating npmlog soon, so this PR drops that dependency in favor of proc-log + an otherwise dependency-less custom logger that includes the subset of functionality from npmlog used by node-gyp
  • d485f1b Promisifies the build command since which@3.0.0 is promise only
  • 02fc75d Promisifies the remove and clean commands after dropping rimraf in favor of native recursive rm with fs.promises.rm
Commits

Each commit in the PR is a separate unit of work and I'd be happy to break it up if this is too difficult to review as is.

Reference issues/PRs

After doing the above, I looked through the list of open issues and PRs and identified the following that would either be closed or superseded by this PR.

Closes #2753
Closes #2647
Closes #2358
Closes #2357
Closes #2756
Closes #2379
Closes #2225

BEGIN_COMMIT_OVERRIDE
feat!: update engines.node to ^14.17.0 || ^16.13.0 || >=18.0.0
deps: nopt@^7.0.0
feat: replace npmlog with proc-log
deps: standard@17.0.0 and fix linting errors
deps: which@3.0.0
fix: promisify build command
deps: glob@8.0.3
feat: drop rimraf dependency
fix: use fs/promises in favor of fs.promises
END_COMMIT_OVERRIDE

@lukekarrys lukekarrys marked this pull request as ready for review December 9, 2022 21:48
@lukekarrys lukekarrys changed the title Update deps and sync engines with npm Sync deps and engines with npm Dec 9, 2022
@lukekarrys
Copy link
Member Author

#2772 addresses the test failures which are unrelated to the changes in this PR

@lukekarrys
Copy link
Member Author

Also if this PR gets accepted, my other PR (#2771) should be reviewed first as I'd want it to make it into the node-gyp@9 release.

@lukekarrys
Copy link
Member Author

@nodejs/node-gyp friendly ping to review this if possible. there's no timeframe, but this refactor will eliminate all non-deduped dependencies within npm, so it's a change we're excited to ship

@lukekarrys
Copy link
Member Author

Rebased against main to resolve conflicts.

@rvagg
Copy link
Member

rvagg commented Dec 19, 2022

ooof, this is a big changeset, which makes it hard to review; I suspect you're going to have to be very patient trying to get someone to review this, I certainly don't have time to look at this in the immediate future unfortunately. Nice to see all of this done, but doing it all in one go means that the chances of getting it in in a decent timeframe are quite low.

@lukekarrys
Copy link
Member Author

@rvagg i appreciate the comment and agree that the size of the diff is very big.

i can break it up into multiple PRs and will consider the best way to do that in order to make reviewing easier. if you have any thoughts on what you would like to see there, please let me know.

off the top of my head and i could do the dev deps and linting separately since those changes are 90% automated. and the biggest functional difference is the logging which i could break out as well.

the biggest sticking point is that some of the PRs will need to be stacked since for example the logging change can’t be linked with the current version of standard. but if i go that route, i’ll make it clear in what order they should be reviewed.

@ruyadorno
Copy link
Member

Also if it helps you @rvagg I actually took the time to review the changes and it all looks good to me. While I'm not an owner in this repo, hopefully having another collaborator pair of eyes might help.

@lukekarrys
Copy link
Member Author

i'll also add that the commits are ordered and separated very closely to how i would separate them into different PRs, and the ones i linked in the body are the most relevant to a reviewer. but i understand that the github ui leaves something to be desired when reviewing individual commits within a PR

lib/clean.js Outdated Show resolved Hide resolved
lib/clean.js Outdated Show resolved Hide resolved
lib/remove.js Outdated Show resolved Hide resolved
@imatlopez
Copy link
Contributor

@rvagg plans for this? Happy it takes over my promisify prs

@KoenDG
Copy link

KoenDG commented Apr 2, 2023

Sorry to suddenly interrupt, but this PR seems stuck, and a security update here depends on it: #2796 (comment)

Reading lerna/lerna#3605 indicates waiting for review, but scrolling up a but shows code comments that make it look like this review has already happened.

Is github not showing everything?

Regardless, what would be required for this to progress? I'm asking because it's related to a security issue, which if I'm reading it right cannot be resolved until this PR is finished and merged: https://nvd.nist.gov/vuln/detail/CVE-2022-25881

Apologies if this comes off strong, it's not my intent. It's just something I noticed a while back and it seems there's no movement. Thought I'd shake the grapevine a bit. Is there someone who needs to get involved in this that has perhaps lost track of it? It happens.

@rvagg
Copy link
Member

rvagg commented May 1, 2023

Hey @imatlopez are you able to help review this? It really needs someone with the time and experience with this codebase to walk through it and give it the 👍. I'm unlikely to be able to find any time to do this but on the surface it seems like a worthwhile set of changes, just too much.

@rvagg
Copy link
Member

rvagg commented May 1, 2023

I should acknowledge that @ruyadorno also walked through this, that's helpful. It would be good if we had more than one person who's name's familiar around here to give this a tick. This code goes out to a lot of people so we can't ship big changes lightly.

lib/log.js Show resolved Hide resolved
@imatlopez
Copy link
Contributor

@lukekarrys you have conflicts in the pr, @rvagg gave it another pass today and besides those two comment, it looked ok to me.

@lukekarrys
Copy link
Member Author

I will resolve the conflicts and address the comments this week.

@lukekarrys
Copy link
Member Author

This PR has been rebased to fix conflicts with the latest commits in main. Since make-fetch-happen was updated in #2796, I dropped the commit updating that from this PR.

@lukekarrys
Copy link
Member Author

lukekarrys commented May 9, 2023

@imatlopez @rvagg Thank you both for your feedback on this PR. I believe I've addressed all the feedback presented so far and fixed the conflicts.

CI was previously failing on what I believe was a flaky test, but I will wait and watch the currently running CI on this PR to ensure it all passes.

legobeat added a commit to legobeat/node-gyp that referenced this pull request May 17, 2023
This reverts commit 02480f6, thereby
rolling back dependency make-fetch-happen from ^11.0.3 to ^10.0.3.

The upgrade is breaking for node-fetch users as it has transitive
dependencies with syntax incompatible with supported Node.js versions.

Related:
- nodejs#2770
- nodejs#2837
- nodejs#2816
- nodejs#2848
- nodejs#2827
- nodejs#2796
@JamesHenry
Copy link

Thanks for all your work on this @lukekarrys!

Looking forward to this landing!

@imatlopez
Copy link
Contributor

@lukekarrys seems this has new conflicts @rvagg once those are resolved is merge on the table?

@lukekarrys
Copy link
Member Author

Thanks for the heads up @imatlopez! I rebased this PR to fix all the conflicts. Note that the original body of the PR is also updated with the latest commit links.

The only real change to fix the conflicts was to drop the commit updating tap since this uses mocha now as a test runner.

@imatlopez
Copy link
Contributor

I'd imagine tap would be preferred now that it comes with node but long live mocha?

2 windows tests are failing (but not all so flaky?)

@cclauss
Copy link
Contributor

cclauss commented Jun 14, 2023

Re-running the two failed visual studio jobs.

@JamesHenry
Copy link

@cclauss Thanks a lot for rerunning, looks like this is good to go now?

(This issue indirectly causes a warning on every fresh lerna repo, that's why I'm particularly invested in its outcome 😄)

@cclauss
Copy link
Contributor

cclauss commented Jun 20, 2023

@JamesHenry
Copy link

@cclauss I'm not part of the nodejs org, nor remotely familiar with this codebase, so I don't think my review carries much weight here! I'm just happy to see a resolution and if @lukekarrys is happy with it that's good enough for me

@cclauss cclauss merged commit 192eec2 into nodejs:main Jun 20, 2023
22 checks passed
@lukekarrys lukekarrys mentioned this pull request Jun 24, 2023
@lukekarrys lukekarrys deleted the engines-and-deps branch June 24, 2023 00:15
@lukekarrys
Copy link
Member Author

The plan is to release this as v10.0.0 sometime in the next couple weeks. Thanks for your patience @JamesHenry.

legobeat added a commit to legobeat/node-gyp that referenced this pull request Jun 26, 2023
This reverts commit 02480f6, thereby
rolling back dependency make-fetch-happen from ^11.0.3 to ^10.0.3.

The upgrade is breaking for node-fetch users as it has transitive
dependencies with syntax incompatible with supported Node.js versions.

Related:
- nodejs#2770
- nodejs#2837
- nodejs#2816
- nodejs#2848
- nodejs#2827
- nodejs#2796
lukekarrys added a commit that referenced this pull request Oct 27, 2023
feat!: update engines.node to ^14.17.0 || ^16.13.0 || >=18.0.0
deps: nopt@^7.0.0
feat: replace npmlog with proc-log
deps: standard@17.0.0 and fix linting errors
deps: which@3.0.0
fix: promisify build command
deps: glob@8.0.3
feat: drop rimraf dependency
fix: use fs/promises in favor of fs.promises
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants