-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Conversation
9b293b4
to
e33ee18
Compare
#2772 addresses the test failures which are unrelated to the changes in this PR |
Also if this PR gets accepted, my other PR (#2771) should be reviewed first as I'd want it to make it into the |
e33ee18
to
6b5651c
Compare
@nodejs/node-gyp friendly ping to review this if possible. there's no timeframe, but this refactor will eliminate all non-deduped dependencies within |
6b5651c
to
665e446
Compare
Rebased against |
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. |
@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 |
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. |
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 |
@rvagg plans for this? Happy it takes over my promisify prs |
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. |
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. |
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. |
@lukekarrys you have conflicts in the pr, @rvagg gave it another pass today and besides those two comment, it looked ok to me. |
I will resolve the conflicts and address the comments this week. |
8c4a100
to
98086ab
Compare
This PR has been rebased to fix conflicts with the latest commits in |
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
Thanks for all your work on this @lukekarrys! Looking forward to this landing! |
@lukekarrys seems this has new conflicts @rvagg once those are resolved is merge on the table? |
- this also promiisifies the build command
98086ab
to
0f75796
Compare
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 |
I'd imagine 2 windows tests are failing (but not all so flaky?) |
Re-running the two failed visual studio jobs. |
@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 😄) |
@JamesHenry , @KoenDG Please |
@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 |
The plan is to release this as v10.0.0 sometime in the next couple weeks. Thanks for your patience @JamesHenry. |
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
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: [email protected] and fix linting errors deps: [email protected] fix: promisify build command deps: [email protected] feat: drop rimraf dependency fix: use fs/promises in favor of fs.promises
* 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: [email protected] and fix linting errors * deps: [email protected] - this also promiisifies the build command * deps: [email protected] * feat: drop rimraf dependency * fix: use fs/promises in favor of fs.promises
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: [email protected] and fix linting errors deps: [email protected] fix: promisify build command deps: [email protected] feat: drop rimraf dependency fix: use fs/promises in favor of fs.promises
Checklist
npm install && npm test
passesDescription 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 thenpm/cli
source there are a number of dependencies that end up nested withinnode-gyp/node_modules
due to conflicting versions.To accomplish this a number of other changes were made:
fe5b13a
BREAKING CHANGE updateengines.node
to^14.17.0 || ^16.13.0 || >=18.0.0
a729c15
Updatestandard
to work with newer JS syntax and fix new lint errors9ccc66d
The @npm/cli-team team will be deprecatingnpmlog
soon, so this PR drops that dependency in favor ofproc-log
+ an otherwise dependency-less custom logger that includes the subset of functionality fromnpmlog
used bynode-gyp
d485f1b
Promisifies thebuild
command since[email protected]
is promise only02fc75d
Promisifies theremove
andclean
commands after droppingrimraf
in favor of native recursive rm withfs.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: [email protected] and fix linting errors
deps: [email protected]
fix: promisify build command
deps: [email protected]
feat: drop rimraf dependency
fix: use fs/promises in favor of fs.promises
END_COMMIT_OVERRIDE